Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] [WIP] Add documentation for form type extensions #1691

Closed
wants to merge 10 commits into from
Closed

[Form] [WIP] Add documentation for form type extensions #1691

wants to merge 10 commits into from

Conversation

pvanliefland
Copy link

This is sill pretty much WIP. As suggested in issue #814, this is a proposal for a cookbook articles form form type extensions.

Still need to :

  • Check if the example is relevant
  • Update the cookbook TOC
  • Add previous / next links at the bottom of the articles

use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\FormTypeExtensionInterface;

class ImageTypeExtension extends AbstractTypeExtension {
Copy link
Member

Choose a reason for hiding this comment

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

the curly brace should be on the next line according to the Symfony2 CS.

@pvanliefland
Copy link
Author

Fixed

*/
public function getWebPath()
{
// return the full image url, to be used in templates for example
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to add ... before the sentence or, maybe better, something like this:

// the full image url, to be used in templates for example
return ...;

Copy link
Author

Choose a reason for hiding this comment

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

I did something in that spirit, hope it's ok

@weaverryan
Copy link
Member

Hi Pierre!

This is a very nice entry on a great topic - thanks for writing it! I've just added some notes in the article - I should be able to merge this in when you've checked those out. Obviously, this article applies only to 2.1 and higher (not 2.0). That may or may not be fine - we haven't decided yet exactly what our support for 2.0 will look like and for how long. I'll let you know either way.

For the table-of-contents and next-previous links, you just need to add an entry in cookbook/map.rst.inc and cookbook/form/index.rst - the first is for visual displaying and the second is for internal indexing (which is responsible for next-previous).

Thanks!

@pvanliefland
Copy link
Author

Hello Ryan,

Thanks a lot for the feedback. I've implemented almost all your suggestions, except for the "we" part (see comment above) and for the upload directory issue (see comment above). Once you've taken your final decision for those two last issues, I'll do a last commit, and maybe squash the commits into a single one ?

For the 2.0 branch, no problem, it will be trivial to rework the article so that it works with 2.0. Let me know when you have taken a decision.

I've added entries in map.rst.inc and in index.rst.inc, next to the "create a custom form field type" entry - hope it's ok.

Cheers,

Pierre

@weaverryan
Copy link
Member

Hey Pierre!

Thanks for making all the changes - awesome! Let me figure out what we want to do with 2.0 and I'll let you know.

Thanks!

@pvanliefland
Copy link
Author

Ryan,

My last commit takes care of the first person plural thing, and corrects a few other errors or inconsistencies I spotted while reading the whole thing one last time.

One last doubt : I always use "field type extension" and not "form type extension". I haven't noticed before, maybe because it feels more natural that way, but I'm afraid it's incorrect. What do you think ?

Pierre

@weaverryan
Copy link
Member

Hey Pierre!

Ok, I have some word on this :). We are going to continue to support 2.0, at least for awhile longer. So, could you please update the PR to work for 2.0 (e.g. change setDefaultOptions to getDefaultOptions). When you do that, I'll patch this into the 2.0 branch (if you want to create a new PR against 2.0, that would be great - but I can also just patch from this PR) and then update it for 2.1 and merge it upstream.

Thanks!

@pvanliefland
Copy link
Author

Hep Ryan,

Ok, I'll do it this week. What's your decision about the "field type extension" or "form type extension" thing (see above) ?

Pierre

@weaverryan
Copy link
Member

Ah, good question. I think "form type extension" is more proper, so my vote is for that.

Thanks!

@pvanliefland
Copy link
Author

@weaverryan : the last commit takes care of the "form type extension" change. I think this PR is ready to be merged.
I created a separate PR (#1806) with the adapted code for 2.0.x, it will be easier for you, because there were a few more changes than just setDefaultOptions -> getDefaultOptions. I tested the adapted code thoroughly so there shouldn't be any surprise.

Cheers,

Pierre

@pvanliefland
Copy link
Author

PS : do you want me to squash the commits for this PR ?

@pvanliefland
Copy link
Author

The last two commits take the remarks made by @wouterj in #1806 into account. Let me know when everything is ok so that I can squash everything into a single commit.

@weaverryan
Copy link
Member

Closing this as #1806 was merged in and updated (using this PR as a guide) to 2.1 at sha: 28ea82b

@weaverryan weaverryan closed this Oct 24, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants