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

Don't use deprecated functions in Callback.rst #4105

Closed
wants to merge 3 commits into from
Closed

Don't use deprecated functions in Callback.rst #4105

wants to merge 3 commits into from

Conversation

Rootie
Copy link
Contributor

@Rootie Rootie commented Aug 8, 2014

Q A
Doc fix? yes
New docs? no
Applies to 2.2+
Fixed tickets

The addViolationAt function is marked as deprecated and should be replaced by buildViolation according to the Symfony update guides.

The addViolationAt function is marked as deprecated and should be replaced by buildViolation according to the Symfony update guides.
@xabbuh xabbuh mentioned this pull request Aug 8, 2014
@xabbuh
Copy link
Member

xabbuh commented Aug 8, 2014

This should be merged into the 2.5 branch.

@xabbuh
Copy link
Member

xabbuh commented Aug 8, 2014

@Rootie Can you also update the call in /cookbook/validation/custom_constraint.rst?

@stof
Copy link
Member

stof commented Aug 8, 2014

Actually, the doc should show code compatible with both the 2.4 and 2.5 API. Otherwise bundle authors might not maintain compatibility with both (for projects, you can skip this need as you control which API is used).

See symfony/symfony#11587 for the way it is done in Symfony

@xabbuh
Copy link
Member

xabbuh commented Aug 8, 2014

That's a valid point. But I think that could well be done in a new cookbook chapter. We can then add a reference to this entry here.

@Rootie You would also have to add a versionadded directive which explains the deprecation.

@stof
Copy link
Member

stof commented Aug 8, 2014

Well, if you only show the 2.5 API here, I'm sure people will create bundles incompatible with one of the API. Symfony currently supports 3 validation APIs:

  • 2.4: the 2.4- API, which is still there for BC
  • 2.5: the new backward incompatible API introduced in 2.5
  • 2.5-BC: the 2.5 API with an additional BC layer reproducing the 2.4 API, but usable only for PHP 5.3.9+ (due to a limitation in previous PHP versions).

The default config is to use 2.5-BC for PHP 5.3.9+ and to use 2.4 for old 5.3 versions (we cannot switch automatically to the incompatible API).
This means that if your bundle uses the 2.4 API (addValidationAt being part of it), it will be broken for people opting explicitly for the non-BC API. On the other hand, if you use the new API (buildViolation), it will make your bundle incompatible with the default config of FrameworkBundle for some PHP version, and will also make it incompatible with the 2.3 LTS.
given that writing code compatible with both APIs is not that complex, it is better to write the documentation the right way

@xabbuh
Copy link
Member

xabbuh commented Aug 8, 2014

Well, if you think it's not too hard, we should do that. I didn't dive that deep into the changes. Nonetheless, having a section explaining all differences in detail seems valuable to me. But that's out of scope of this pull request and #4094 also addresses that.

@xabbuh
Copy link
Member

xabbuh commented Aug 8, 2014

Just noticed: there's also an older pull request for this (#4056).

@weaverryan
Copy link
Member

@stof Thank you very much for the detailed description - I was hoping someone could help us understand this.

But wow, this is a bit nuts! :) To simplify things, we'll obviously only be making changes to the 2.5+ version of the docs. 2.4+2.3 use the 2.4 API, so they'll stay the same.

For 2.5+, I agree with Stof that we should show both. But instead of writing code that's truly compatible with everything, I think we should just show both with comments saying which is which:

// If you're using the new 2.5 validation API (you probably are!)
$context->buildViolation('This name sounds totally fake!')
    ->atPath('firstName')
    ->addViolation();

// If you're using the old 2.4 validation API
$context->addViolationAt(
    'firstName',
    'This name sounds totally fake!',
    array(),
    null
);

For a normal user, telling them to have an if statement with some duplicated code in order to support both API's is just a bad experience. But I think this accomplishes it as well as we can. Like @xabbuh mentioned, I think we should have a cookbook article explaining what's going on with all of this :).

Thoughts?

@Rootie
Copy link
Contributor Author

Rootie commented Aug 8, 2014

Meanwhile i also updated custom_constraint.rst and added a versionadded hint.

To me having code that is already marked as deprecated in the referecne seems a little strange.
A new cookbook entry sounds good to me too. The two files modified here can reference to that entry with a note like "Public bundles should also support the old api. For details see ."

@stof
Copy link
Member

stof commented Aug 12, 2014

@Rootie The 2.4 API is marked as deprecated. But given that it is still possible to have a project using the 2.4 API only (because the 2.5-bc API requires PHP 5.3.9+ while Symfony allows 5.3.3+), an open-source bundle must still be compatible with it.

and yes, I agree we could keep the instanceof check to support both together in a cookbook article (but we should add a warning linking to it)

@weaverryan
Copy link
Member

@Rootie Thanks for those tweaks. Can you also update the code to show the old and new syntax. See Stof's comment about it - we must show the deprecated version because if you're not on at least PHP 5.3.9, then you can't easily upgrade your code to the 2.5 API (since the 2.5-BC API requires PHP 5.3.9+).

Thanks!

@weaverryan
Copy link
Member

Hey @Rootie

I've taken your commit and moved it to #4233 so we can summarize all the changes. Thanks for starting this! I'll close this PR.

@weaverryan weaverryan closed this Sep 16, 2014
weaverryan added a commit that referenced this pull request Oct 18, 2014
…, weaverryan)

This PR was merged into the 2.5 branch.

Discussion
----------

2.5 Validation API changes

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | 2.5
| Fixed tickets | #4094

Hi guys!

This takes the work in #4056, #4105 and #4161 and extends it based on some feedback. Basically, the whole validation stuff is quite difficult, because:

A) We want to show the non-deprecated methods so people use the new stuff
B) We don't want to confuse users on the 2.4 API (even though this number of users should be very small, as it would require you to have started a project in the past and be using 5.3.9 and lower #4105 (comment))

This solution is to show the new way, but always show the old way in comments. This would check of A and B in #4094.

Thanks!

Commits
-------

9874d8e [#4233][#4094] Making validateValue and validate changes
94fc520 Minor tweaks and a missing location thanks to xabbuh and WouterJ
f97ba7a Fixes thanks to @xabbuh
279d8d6 Adding a section about keeping BC in a re-usable bundle
280440e Adding details about the 2.4 API as comments
e658b56 added a versionadded comment to Callback.rst
70c5ca1 Update custom_contraint.rst to meet the new 2.5 api
5dfe499 Don't use deprecated functions in Callback.rst
f4380ed Update Callback.rst
042dcf9 Replace addViolationAt (deprecated) by buildViolation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants