Adding support for multilang #33

dantleech · 2013-10-26T07:45:56Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#32
License	MIT
Doc PR	pending

This PR adds multilang support to RoutingAuto

dantleech · 2013-10-26T07:46:21Z

Currently depends on: doctrine/phpcr-odm#372

lsmith77 · 2013-10-26T15:28:31Z

cool .. still in the Ukraine so not sure yet when I can help review. one thing though .. we should try to update this Bundle soon to our standards to prevent more people from suffering from BC breaks.

dantleech · 2013-10-26T15:42:26Z

Probably ... I am somewhat reluctant to do it atm as it will probably
be quite time consuming.

I am planning a medium refactor to enable (the possiblity at least) for
ORM support, but this should largely keep BC, but maybe it would make
sense to boost it to the bundle standards then.

On Sat, Oct 26, 2013 at 08:28:32AM -0700, Lukas Kahwe Smith wrote:

cool .. still in the Ukraine so not sure yet when I can help review. one
thing though .. we should try to update this Bundle soon to our standards
to prevent more people from suffering from BC breaks.

—
Reply to this email directly or [1]view it on GitHub.

References

Visible links

Adding support for multilang #33 (comment)

dantleech · 2013-10-26T19:00:33Z

ok -- this does not seem to be possible with the PHPCR-ODM at the moment, at least in the way that I am doing this.

The RoutingAutoBundle gets the scheduled inserts and updates determines if there are any candidates for "auto routing" and then creates the auto routes and adds them to the insert/update schedule. So far so good.

Problem: To retrieve the translated fields of the subject of the "auto routing" PHPCR-ODM requires that the document be persisted.

There are two solutions I can think of:

findTranslation can also take into account bound translations which have not yet been persisted.
We can add a post flush actions queue enabling listeners to add actions after the flush has "completely" finished.
- There is the postFlush event but I am 99% sure that this will not work in this situation.

In anycase, doing anything after the flush would be sub optimal.

dbu · 2013-10-28T15:36:56Z

+1 for moving to bundle standards and working towards a stable release.

post flush would be sad, we would need another flush, and i think
postflush is exactly the place where issuing another flush gets really
hairy.

i commented on the phpcr-odm issue that i think getLocalesFor and
findTrnaslation should work for non-persisted documents as well. i
consider it a bug that those do not work.

dantleech · 2013-11-02T08:50:49Z

EventListener/AutoRouteListener.php

+
+                        if ($route instanceof AutoRoute) {
+                            $routeParent = $route->getParent();
+                            $id = spl_object_hash($routeParent).$route->getName();


This is a potential edge case fail. Current there is no way to reliably retrieve the ID of a non-persisted document. The AutoRoute uses setParent and setName (which allows renaming) as opposed to setId (which causes a fail because the ID is immutable).

This means that I cannot use $metadata->getIdentifierValue. Here I get the object hash of the parent and append the name of the autoroute.

you should use $dm->getUnitOfWork()->getObjectIdentifier($obj) and not the metadata method.

we will deprecate the metadata->getIdentifierValue method in phpcr-odm 1.1, it makes little sense (its not even guaranteed that the id is mapped to the document, as parent and name completely identify the document). see also doctrine/phpcr-odm#368

Its getUnitOfWork()->getDocumentId() - but this doesn't work here as the Route documents are generated each time (they are not managed, they are NEW) and the getDocumentId method only works for Managed documents.

okay, i got confused by your initial comment i think ;-)
you do not build the object id anyways, that would be the repository path. the uow method that indeed is called getDocumentId gets you the path by document object, using the spl_object_hash. so you use this to generate a random id, correct? i think spl_object_hash is a bad random id, as its basically the memory pointer to the object. should you not rather just use rand() here, to reduce the probability of a clash as much as possible?

No, its not for a random ID - it needs to be an identifier which represents the path of the generated route.

The builder now iterates over each of the translations and generates a route stack for each. Therefore in the collection route stacks there is a very good chance of having route objects with duplicate paths. e.g.

/this/is/a/path/to/fr/article-en-francais /this/is/a/path/to/en/article-in-english

The route representing /this/is/a/path appears in both stacks but it needs (and has to be) persisted only once.

seems to be too early for me. you hash the parent. so you would hash the same object and would want to get the overlaps. fine then.

dantleech · 2013-11-02T08:51:27Z

ok... this works! Depends on : doctrine/phpcr-odm#375

dbu · 2013-11-04T18:27:26Z

i commented on the phcpr-odm PR. i think it is really going in the right direction. if we can wrap the phpcr-odm fix up this week, i can tag 1.0.1 of phpcr-odm later this week.

dbu · 2013-11-17T09:36:55Z

EventListener/AutoRouteListener.php

+                            $id = spl_object_hash($routeParent).$route->getName();
+                        } else {
+                            $metadata = $dm->getClassMetadata(get_class($route));
+                            $id = $metadata->getIdentifierValue($route);


okay, and as the route happens to have the id field mapped, this will work. nonetheless, we probably will deprecate this method in favor of $dm->getUnitOfWork->getDocumentId(). why don't you simply do $route->getId()? as you know the class of $route, you know that method is existing.

Yeah, this works for Route but not for AutoRoute which uses the parent strategy. The problem is that I cannot gurantee, here, that the Route is a specific class. It could be anything as the user can implement their own Maker classes.

unless you have the identifier mapped on your class, the metadata->getIdentifierValue method will not be able to return you something.

isn't uow getDocumentId populated as soon as persist was called?

I don't think that these routes are persisted at this point. I would need a way to infer the path from the strategy.

bertterheide · 2013-12-16T08:28:54Z

What's the state of this issue? Will this be fixed in a feature release?

dantleech · 2013-12-16T11:04:31Z

@bertjh we are waiting on doctrine/phpcr-odm#375 - which is very nearly done, after that this is fine.

bertterheide · 2013-12-20T10:24:03Z

@dantleech Any idea about a working timeframe?

dantleech · 2013-12-20T10:50:28Z

@bertjh well, theoretically the Doctrine PR just needs a rebase and merge. I just need to sit down and work on this -- luckily I start my vacation tomorrow so if all goes well it should certainly be OK next week.

bertterheide · 2013-12-20T12:14:30Z

@dantleech Great, don't work to hard and have a nice Christmas!

dantleech · 2013-12-23T13:00:09Z

composer.json

@@ -13,7 +13,8 @@
    "require": {
        "symfony-cmf/routing-bundle": "1.1.*",
        "symfony-cmf/core-bundle": "1.0.*",
-        "aferrandini/urlizer": "1.0.*"
+        "aferrandini/urlizer": "1.0.*",
+        "doctrine/phpcr-odm": "dev-master as 1.0.x-dev"


This PR needs dev-master of PHPCR-ODM -- how should we handle that?

I think that is ok like this .. we will, just like for 1.0, first release phpcr, then jackalope, then phpcr odm and only then the bundles

should we not depend on phpcr-odm 1.1.* with a minimum stability of dev? everything should be ok with phpcr-odm 1.1 or if not we need to fix that.

then we should use 1.1.*@dev, we should not use unstable versions for all packages because one needs an unstable version.

ok, will change to @wouterj 's suggestion.

Actually, that doesn't work as testing relies on 1.0 -- I think leaving it as "as 1.0.x-dev" is best for now. This bundle isn't stable yet.

dantleech · 2013-12-23T13:05:34Z

This is working now with dev-master phpcr-odm and is good for me except:

We need to figure out how to manage the phpcr-odm version
We need to wait for Routing auto rewrite symfony-cmf-docs#353 before updating the docs.

[WIP] Adding support for multilang

dantleech · 2013-12-27T15:03:44Z

Pop.

wouterj · 2013-12-27T15:05:14Z

Thank you! Now only #37 needs to be finished before we can refactor to bundle standards and tag stable, isn't it?

dantleech · 2013-12-27T15:12:41Z

Yep.. my next job :)

bertterheide · 2013-12-30T09:55:59Z

@dantleech How can I import this commit in my composer.json? I tried adding a specific commit ( "symfony-cmf/routing-auto-bundle": "dev-master#e325fed1ee45aa903519778a186685712ce657fb" ) but then it starts complaining about mismatching phpcr-odm versions.

  Problem 1
    - symfony-cmf/testing 1.0.x-dev requires doctrine/phpcr-odm 1.0.* -> satisfiable by doctrine/phpcr-odm[1.0.x-dev, 1.0.0, 1.0.0-RC1, 1.0.0-RC2, 1.0.0-RC3, 1.0.0-RC4, 1.0.0-RC5, 1.0.0-alpha1, 1.0.0-alpha2, 1.0.0-beta1, 1.0.0-beta2, 1.0.0-beta3, 1.0.0-beta4, 1.0.0-beta5, 1.0.1].
    - symfony-cmf/testing 1.0.0 requires doctrine/phpcr-odm 1.0.* -> satisfiable by doctrine/phpcr-odm[1.0.x-dev, 1.0.0, 1.0.0-RC1, 1.0.0-RC2, 1.0.0-RC3, 1.0.0-RC4, 1.0.0-RC5, 1.0.0-alpha1, 1.0.0-alpha2, 1.0.0-beta1, 1.0.0-beta2, 1.0.0-beta3, 1.0.0-beta4, 1.0.0-beta5, 1.0.1].
    - symfony-cmf/testing 1.0.x-dev requires doctrine/phpcr-odm 1.0.* -> satisfiable by doctrine/phpcr-odm[1.0.x-dev, 1.0.0, 1.0.0-RC1, 1.0.0-RC2, 1.0.0-RC3, 1.0.0-RC4, 1.0.0-RC5, 1.0.0-alpha1, 1.0.0-alpha2, 1.0.0-beta1, 1.0.0-beta2, 1.0.0-beta3, 1.0.0-beta4, 1.0.0-beta5, 1.0.1].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.x-dev].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-RC1].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-RC2].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-RC3].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-RC4].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-RC5].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-alpha1].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-alpha2].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-beta1].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-beta2].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-beta3].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-beta4].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.0-beta5].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.1].
    - Can only install one of: doctrine/phpcr-odm[dev-master, 1.0.x-dev].
    - symfony-cmf/routing-auto-bundle dev-master requires doctrine/phpcr-odm dev-master as 1.0.x-dev -> satisfiable by doctrine/phpcr-odm[dev-master].
    - Installation request for symfony-cmf/routing-auto-bundle dev-master#e325fed1ee45aa903519778a186685712ce657fb -> satisfiable by symfony-cmf/routing-auto-bundle[dev-master].
    - Installation request for symfony-cmf/testing 1.0.* -> satisfiable by symfony-cmf/testing[1.0.0, 1.0.x-dev].

dantleech · 2013-12-30T10:13:28Z

@bertjh this requires a fix scheduled for the 1.1 release of PHPCR-ODM, other CMF components depend on version 1.0 currently. You can include the latest version (or any version you want) using an alias:

'require': {
    'doctrine/phpcr-odm': 'dev-master as 1.0'
}

See: http://getcomposer.org/doc/articles/aliases.md

bertterheide · 2013-12-30T10:26:56Z

@dantleech Thanks for the info. The fix was adding PHPCR-ODM to my composer.json.

 "doctrine/phpcr-odm": "1.0.x-dev as dev-master",

bertterheide · 2013-12-31T08:56:17Z

@dantleech When will the other CMF components be updated to support #33?

wouterj · 2013-12-31T08:58:17Z

@bertjh when CMF 1.1 will be released I think.

bertterheide · 2013-12-31T09:00:26Z

@wouterj What's the planning on that? I'm trying to build my own CMS, and this is kind of essential.

wouterj · 2013-12-31T09:02:14Z

@bertjh I thought we planned in the end of January

bertterheide · 2013-12-31T10:57:31Z

@wouterj Our deadline is the 1st of Februari. Is it possible to speed this up a little?

wouterj · 2013-12-31T11:03:46Z

@bertjh doctrine phpcr-odm is planned to release 1.1 in 12 days (https://github.com/doctrine/phpcr-odm/issues?milestone=2 ). I think Sonata admin will be updated to doctrine 1.1 after that release and then you can use the master version of Sonata admin and everything should work.

bertterheide · 2013-12-31T11:04:58Z

@wouterj OK. I'll wait for that. Fijne jaarwisseling!

dantleech mentioned this pull request Oct 28, 2013

Retrieve locales for non-persisted documents doctrine/phpcr-odm#372

Closed

dantleech reviewed Nov 2, 2013
View reviewed changes

dbu reviewed Nov 17, 2013
View reviewed changes

dantleech mentioned this pull request Nov 18, 2013

[RFC] Proposal for new configuration format. #38

Closed

Adding support for multilang

d07b40b

dantleech reviewed Dec 23, 2013
View reviewed changes

dantleech added a commit that referenced this pull request Dec 27, 2013

Merge pull request #33 from symfony-cmf/multilang_refactoring

e325fed

[WIP] Adding support for multilang

dantleech merged commit e325fed into master Dec 27, 2013

wouterj deleted the multilang_refactoring branch December 27, 2013 15:04

Adding support for multilang #33

Adding support for multilang #33

Conversation

dantleech commented Oct 26, 2013

dantleech commented Oct 26, 2013

lsmith77 commented Oct 26, 2013

dantleech commented Oct 26, 2013

dantleech commented Oct 26, 2013

dbu commented Oct 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech commented Nov 2, 2013

dbu commented Nov 4, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertterheide commented Dec 16, 2013

dantleech commented Dec 16, 2013

bertterheide commented Dec 20, 2013

dantleech commented Dec 20, 2013

bertterheide commented Dec 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech commented Dec 23, 2013

dantleech commented Dec 27, 2013

wouterj commented Dec 27, 2013

dantleech commented Dec 27, 2013

bertterheide commented Dec 30, 2013

dantleech commented Dec 30, 2013

bertterheide commented Dec 30, 2013

bertterheide commented Dec 31, 2013

wouterj commented Dec 31, 2013

bertterheide commented Dec 31, 2013

wouterj commented Dec 31, 2013

bertterheide commented Dec 31, 2013

wouterj commented Dec 31, 2013

bertterheide commented Dec 31, 2013