Skip to content

Fix #250: Replace form type object with classname string#253

Closed
bocharsky-bw wants to merge 4 commits into
symfony:masterfrom
bocharsky-bw:form-type
Closed

Fix #250: Replace form type object with classname string#253
bocharsky-bw wants to merge 4 commits into
symfony:masterfrom
bocharsky-bw:form-type

Conversation

@bocharsky-bw

Copy link
Copy Markdown
Contributor

No description provided.

@javiereguiluz

Copy link
Copy Markdown
Member

@bocharsky-bw thanks for this fix ... but I'm afraid I have bad news for you 😢 This project is compatible with php >=5.3.9 so we can't use the ::class trick and we must use the entire fully qualified class name of the form types.

@bocharsky-bw

Copy link
Copy Markdown
Contributor Author

For now, but when we update to the Symfony 3 we should use 5.5.9 at least, right? It will work then.

@javiereguiluz

Copy link
Copy Markdown
Member

Yes.

@bocharsky-bw

Copy link
Copy Markdown
Contributor Author

So keep it as is or update with fully qualified classname?

@javiereguiluz

Copy link
Copy Markdown
Member

Good question. I'd prefer to use the full string because that works perfectly on 2.8 and 3.0 ... but in the future we'll need to change it again to make code more concise.

@voronkovich

Copy link
Copy Markdown
Contributor

@bocharsky-bw

Copy link
Copy Markdown
Contributor Author

@javiereguiluz You're right. I updated with fully qualified classname for now.

@bocharsky-bw

Copy link
Copy Markdown
Contributor Author

@voronkovich Did you mean this https://github.com/symfony/symfony-demo/blob/master/src/AppBundle/Form/PostType.php#L64 ? Seems for now we should use fully qualified classnames, but thanks you pointed it!

@voronkovich

Copy link
Copy Markdown
Contributor

@bocharsky-bw, ok.

@stof

stof commented Nov 18, 2015

Copy link
Copy Markdown
Member

You should also switch to FQCN rather than legacy type names inside the existing form types to avoid deprecation warnings there

@stof

stof commented Nov 18, 2015

Copy link
Copy Markdown
Member

@javiereguiluz this requires bumping the Symfony requirement to ~2.8 though. Currently, the code still says ~2.7

@bocharsky-bw

Copy link
Copy Markdown
Contributor Author

Switched. Thanks @stof !

Comment thread app/config/services.yml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bocharsky-bw now we needn't this service. You can remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bocharsky-bw, because in the PostType we use FQCN.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it's true. Thanks!

@yceruto

yceruto commented Nov 20, 2015

Copy link
Copy Markdown
Member

This comment is not related to this PR. But IMHO these changes are a throwback for developers. :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This summary field is a string type, why use textarea? or perhaps we should redefine it as text?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't necessary to change it to text... Even with 255 length more convenient work with textarea.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bocharsky-bw then, when users to type a text with 256 characters or more will be truncated and this could be an unexpected result for the user. I suggest then use maxlength 255 for this widget.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_length is deprecated option

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@voronkovich I meant to the html attribute:

'attr' => array('maxlength' => 255)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yceruto, ok.

@javiereguiluz

Copy link
Copy Markdown
Member

@bocharsky-bw thanks for working on this. I'm merging it because I want to release a Symfony 2.8-based demo app soon. Thanks!

@bocharsky-bw bocharsky-bw deleted the form-type branch December 10, 2015 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants