This repository has been archived by the owner on Feb 24, 2023. It is now read-only.
Change apply method to always return a boolean value. This makes converti #56
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The semantic of the converters is also changed with this patch. Currently, we only convert parameters that are not yet in the attributes. After the patch, you convert all parameters even if they are already in the request attributes. |
|
The problem with having to abort the parameter conversion when the attribute is already there is if you give a route a default it will already be present and not converted. |
|
And the conversion of a parameter is aborted if a converter returns true, the converter should be able to handle default parameters :) |
|
As you might want to load a default category object if no specific category was provided. |
fabpot
added a commit
that referenced
this pull request
Nov 25, 2011
Commits ------- 9fcdbfd Merge pull request #1 from henrikbjorn/DoctrineManagerRegistry 07ba5da doctrine autoloading e921615 change class_exists to interface_exists 2f92111 mark test as skipped if doctrine managerregisty is not availible 8c73373 Ignore vendor dir d095d21 add doctrine common autoloading 193389d Add standalone testing autoloading 9ebe009 Refactored ParamConverter and add some tests da8dd75 Refactor Doctrine ManagerRegistry to be generic Discussion ---------- Doctrine Manager Registry Refactored the DoctrineParamConverter to use the Common Registry. Also refactored some other parts of the converter to use a more clean part of the Doctrine API. Problem now is, this would work with MongoDB and the other ODMs, however its bound against the doctrine service. I am not sure how we can fix that. Essentially we could make it configurable, but then it would still only work with exactly one registry. --------------------------------------------------------------------------- by fabpot at 2011/11/23 02:19:45 -0800 The tests do not pass for me. I have updated all Doctrine repositories to origin/master. --------------------------------------------------------------------------- by beberlei at 2011/11/23 05:13:06 -0800 How do they fail? Sometihng with the mocks? --------------------------------------------------------------------------- by fabpot at 2011/11/23 05:32:38 -0800 Fatal error: Call to undefined method Mock_ManagerRegistry_44700eba::getManager() in .../vendor/bundles/Sensio/Bundle/FrameworkExtraBundle/Request/ParamConverter/DoctrineParamConverter.php on line 68 --------------------------------------------------------------------------- by henrikbjorn at 2011/11/23 06:00:27 -0800 With the registry breaking BC can #56 be merged aswell ? i would happily do a update to DoctrineParamConverter to make it compatible --------------------------------------------------------------------------- by fabpot at 2011/11/23 06:05:58 -0800 @henrikbjorn: yes, I will merge it just after this one on the master branch. --------------------------------------------------------------------------- by henrikbjorn at 2011/11/23 06:11:24 -0800 @fabpot sweet, can you ping me first then i can have the doctrine converter ready when its merged --------------------------------------------------------------------------- by henrikbjorn at 2011/11/24 06:50:07 -0800 @fabpot should be okay to merge now :). The reason for the error was that it couldnt find ManagerRegistry class when doing the tests. --------------------------------------------------------------------------- by beberlei at 2011/11/25 01:49:05 -0800 If the change by henrik makes your tests work again fabien then something with your Doctrine Common checkout is wrong. Maybe it didnt switch to origin/master correctly and is still on the "local" master? Regarding the "entity_manager" i am unsure to rename to "object_manager". Since this PR only theoretically makes the converter support the ODMs. One solution to support all the ORM, ODMs would be to register a parameter converter per Doctrine ManagerRegistry automatically, i.e. if it finds the bundles or better something with tags (i.e. tag "doctrine.manager_registry")
added 2 commits
November 25, 2011 17:12
…erting attributes with the name name as a action parameter possible
|
@fabpot rebased and tests pass :) good to go. |
fabpot
added a commit
that referenced
this pull request
Nov 25, 2011
Commits ------- 22c69c0 Cleanup 76c521f Change apply method to always return a boolean value. This makes converting attributes with the name name as a action parameter possible Discussion ---------- Change apply method to always return a boolean value. This makes converti Change apply method to always return a boolean value. This makes converting attributes with the name name as a action parameter possible. symfony/symfony#1547 (comment) Before this patch it was not possible to have a /blog/archive/{date} route and a action like `myAction(DateTime $date)` as the request attribute would already exists and the paramconverter would not be called. This path is breaking BC for the apply method on ParamConverterInterface which now MUST return a boolean value indicating if a conversion was done. --------------------------------------------------------------------------- by fabpot at 2011/08/23 02:43:57 -0700 The semantic of the converters is also changed with this patch. Currently, we only convert parameters that are not yet in the attributes. After the patch, you convert all parameters even if they are already in the request attributes. --------------------------------------------------------------------------- by henrikbjorn at 2011/08/23 03:50:06 -0700 The problem with having to abort the parameter conversion when the attribute is already there is if you give a route a default it will already be present and not converted. --------------------------------------------------------------------------- by henrikbjorn at 2011/08/23 03:50:59 -0700 And the conversion of a parameter is aborted if a converter returns true, the converter should be able to handle default parameters :) --------------------------------------------------------------------------- by henrikbjorn at 2011/08/29 06:44:03 -0700 As you might want to load a default category object if no specific category was provided. --------------------------------------------------------------------------- by henrikbjorn at 2011/11/25 08:15:11 -0800 @fabpot rebased and tests pass :) good to go.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change apply method to always return a boolean value. This makes converting attributes with the name name as a action parameter possible.
symfony/symfony#1547 (comment)
Before this patch it was not possible to have a /blog/archive/{date} route and a action like
myAction(DateTime $date)as the request attribute would already exists and the paramconverter would not be called.This path is breaking BC for the apply method on ParamConverterInterface which now MUST return a boolean value indicating if a conversion was done.