make the QB able to automatically handle the attribute translation strategy #353
Conversation
| @@ -167,7 +167,7 @@ public function getTranslationStrategy($key) | |||
| public function getLocaleChooserStrategy() | |||
| { | |||
| if (!isset($this->localeChooserStrategy)) { | |||
| throw new PHPCRInvalidArgumentException("You must configure a language chooser strategy when having documents with the translatable annotation"); | |||
| throw new PHPCRInvalidArgumentException("A language chooser strategy must be configured when mapping documents documents with a translation strategy"); | |||
There was a problem hiding this comment.
just fyi: this will collide a bit with #354 where we cleanup the exceptions.
There was a problem hiding this comment.
so you fixed the wording there already too?
There was a problem hiding this comment.
no, only the class names and namespaces.
There was a problem hiding this comment.
Can you update the exception message there then to something like A language chooser strategy must be configured when mapping documents documents with a translation strategy ?
|
but great initiative, i really think this will be a valuable addition for 1.0! |
| * @return Builder\QueryBuilder | ||
| */ | ||
| public function createQueryBuilder() | ||
| public function createQueryBuilder($locale = null) |
There was a problem hiding this comment.
I would rather that we called added a setLocale method to the QueryBuilder than adding an argument here.
There was a problem hiding this comment.
this would make things quite a lot more complex for example when using this inside sonata admin. with this solution it will automatically just work. otherwise we will have to add explicit code to deal with multilang all over the place.
There was a problem hiding this comment.
How would it make it more complicated? What is the difference between $dm->createQueryBuilder('fr') and $dm->createQueryBuilder()->setLocale('fr'); ?
There was a problem hiding this comment.
the difference is being able to do $dm->createQueryBuilder() for one wants to just use the default locale while still being able to do $dm->createQueryBuilder(false) to disable this behavior entirely.
There was a problem hiding this comment.
I still don't see why this can't be a property of the query builder, why
does it need to spill into the DM? Not calling "setLocale" would be the
same as createQueryBuilder() and equally setLocale(false).
Also, don't forget the createQueryBuilder method of the
DocumentRepository, which, with this change would now require both an
alias and a locale, which might be confusing.
On Fri, Oct 04, 2013 at 01:31:58AM -0700, Lukas Kahwe Smith wrote:
In lib/Doctrine/ODM/PHPCR/DocumentManager.php:
* @return Builder\QueryBuilder */
- public function createQueryBuilder()
- public function createQueryBuilder($locale = null)
the difference is being able to do $dm->createQueryBuilder() for one wants
to just use the default locale while still being able to do
``$dm->createQueryBuilder(false) to disable this behavior entirely.—
Reply to this email directly or [1]view it on GitHub.References
Visible links
There was a problem hiding this comment.
if we do what you propose then it means, we would set the locale to the default locale by default. which in turn means that for disabling or setting a locale other than the default one will need to make this method call. this is kind of annoying since we would then need to determine the default locale first .. which would then be overwritten. and of course the user will need to learn about the existence of this method.
we could of course delay determining the default locale but this will require a try/catch if we want to fetch the locale chooser outside of the DM.
so imho moving this out to me makes the code more complex and also requires more work from the user. optional parameters should be avoided as much as possible. so i agree its not ideal, but imho still better than the alternatives.
There was a problem hiding this comment.
But.. from the POV of an ideal interface, I can't see how the property "locale" is more deserving of a plce in the factory method than any other property. I would rather refactor to make the "ideal" work than regret this later.
There was a problem hiding this comment.
ok .. i have refactored it .. don't really like the result .. but i guess it will solve your concerns
|
so good to merge? |
|
i am +1 once you added a line about the DocumentRepository::createQueryBuilder to the CHANGELOG.md |
| @@ -313,7 +313,7 @@ public function createQuery($statement, $language, $options = 0) | |||
| * | |||
| * @return \Doctrine\ODM\PHPCR\Query\Builder | |||
| */ | |||
| public function createQueryBuilder($selectorName) | |||
| public function createQueryBuilder($selectorName = 'a') | |||
There was a problem hiding this comment.
this is not related to this PR
There was a problem hiding this comment.
Then why is here? I was under the impression that there was a general consensus that we should force the user to choose an alias the Doctrine ORM does?
There was a problem hiding this comment.
lets revert this .. saw this while talking to David .. but its of course a bigger topic
make the QB able to automatically handle the attribute translation strategy
@dbu @dantleech