search

symfony-cmf / routing-auto

RoutingAuto component
https://cmf.symfony.com
Other
6 stars 11 forks source link

Adapter#translateObject() has to return the translated object #53

Closed wouterj closed 9 years ago

wouterj commented 9 years ago

The PhpcrOdmAdapter is already doing this and it is more flexible (not all adapters might be able to things like this by reference).

Kept BC, but triggered a silenced deprecation notices. As this error is not visible, unless you use a custom error handler (like the one showing the deprecation notices in the toolbar), this is a very friendly way to indicate the user might have to change its code. Tests do no longer fail because of this (and adding trigger_error directly is also going to be the standard in Symfony core as of 3.0).

dantleech commented 9 years ago

I guess this makes sense, and I see no reason why the system should work with non-referenced "instances" of the content object.

dbu commented 9 years ago

I see no reason why the system should work with non-referenced "instances" of the content object.

what do you mean by this? this PR is fixing things so that the system now does work with an adapter that needs to return some different object.

wouterj commented 9 years ago

@dantleech immutable objects? This means that when translating the content, it needs to return a new instance (the translated object). There is no way to do this at the moment

dantleech commented 9 years ago

Oh sorry, typo. "I see no reason why the system should not work" :) I was initially just slightly concerned that we havn't tested the system with different instances of a content object, and that perhaps it would create some new issues if we had an adapter that did that.

dbu commented 9 years ago

ah, good then. well, lets see when they come. but the semantics of the interface do make sense to me.

can we rename subjectObject to subject? and document that return statement? otherwise looks good to me.

wouterj commented 9 years ago

I don't like introducing a BC break for renaming a maybe slightly bad named method/property. Let's skip it for 2.0.

Fixed the other comments.

dbu commented 9 years ago

i would deprecate getSubjectObject in favor of getSubject and make the property private. but if you don't want to, i won't insist so @dantleech @WouterJ feel free to merge if you are happy with this.

dantleech commented 9 years ago

I think I would prefer to change it in 2.0: We already have also to rename getAutoRouteTag to getLocale, and I can imagine some other major API changes, so we might as well batch them all up together.