implementing alternate languages for content documents #175

ElectricMaxxx · 2014-07-11T20:55:47Z

Q	A
Bug fix?	[no]
New feature?	[yes]
BC breaks?	[no]
Deprecations?	[no]
Tests pass?	[no]
Fixed tickets	[#166]
License	MIT
Doc PR	not yet

I started to implement that feature. Thought we should take care for the existence of the ManagerRegistry and when it exists the SeoPresentation::setAlternateLocales() method will figure out urls (by UrlGenerator) and pass that information to sonatas PageService. By doing it that way the user just needs to add the right TwigHelper {{ sonata_seo_lang_alternates() }} to see the result. Same procedure as all other metadata comes into the template.

Todo:

implement logic in ContentListener
add registry by compiler pass
add method in SeoPresentation to compute and set the alternate lang values to PageService
tests for unit and frontend
take care on current route
fix strange url problem

ElectricMaxxx · 2014-07-11T22:07:01Z

SeoPresentation.php

+    {
+        foreach ($alternateLocales as $locale) {
+            if ($url = $this->urlGenerator->generate($content, array('_locale' => $locale))) {
+                $this->sonataPage->addLangAlternate($url, $locale);


need to add the current host/domain here

then you simply need to pass true in one of the arguments to generate to make the url absolute.

👍 thank's a lot never know :-)

ElectricMaxxx · 2014-07-12T07:41:44Z

I came to the conclusion at night, to create a kind of a RouteProviderInterface to get RouteCollections Independent from the environment. Otherwise i would end up in endless if-clauses for every env. That Provider should have a ability to configure/set. And i would put the current code into a PhpcrBasedAlternateLocaleRouteProvider.

Edit: started to work on that, hope to push it before soccer match this evening.

dbu · 2014-07-14T09:31:41Z

EventListener/ContentListener.php

+            $documentManager = $this->getDocumentManagerForClass(get_class($content));
+            if ($documentManager
+                && $content instanceof TranslatableInterface
+                && $content instanceof RouteReferrersInterface


you should check for RouteReferrersReadInterface i think.

does the RouteReffersReadInterface? inherit from this on? Our UrlGenerator checks for that interface.

the generator checks for RouteReferrersReadInterface: https://github.com/symfony-cmf/Routing/blob/master/ContentAwareGenerator.php#L126

if the SeoBundle checks for the not-readonly-interface in a pure reading context, this is a mistake and should be relaxed to the ReadInterface.

Oh you are right :-)

ElectricMaxxx · 2014-07-17T10:23:18Z

So ...
i am almost done with that, but i got one problem:

That solution with asking the DocumentManager to serve the contents locale makes sense when doctrine_phpcr exists or the DoctrinePHPCRBundle is there. So i try to load the service, when the doctrine_phpcr is available, but i got trouble. Maybe the tests are cheating me, cause test application got my service when i want it (when the doctrine_phpcr is available), but i did not find the right way of a service definition that depends on an other service/bundle.

Does anybody has some hints?

Tried the follwoing:

compile pass with $container->hasDefenition('doctrine_phpcr)` -> create my service as a separate definition
- same in bundle extension
- loading a phpcr.xml config file when cmf_seo.persistence.phpcr = true with service defintion

maybe i need to show my current state this evening, but maybe there are best practices, including tests that works, cause i think i have got a problem in the configuration test with that bundle loading stuff.

dbu · 2014-07-21T21:28:56Z

i think in such cases we either say you need to enable the service (alternate languages with phpcr-odm in this case) and if you enable without doctrine_phpcr being availalbe, it will simply fail.

the other way we did a couple of places is configuration with true|false|auto, and with auto you would check class_exists(FQN\DoctrinePHPCRBundle) in the DI class and enable if the class exists, disable otherwise.

ElectricMaxxx · 2014-07-22T19:12:02Z

EventListener/ContentListener.php

+                $this->seoPresentation->updateAlternateLocales(
+                    $this->alternateLocaleProvider->createForContent($content)
+                );
+            }
        }


Btw: @wouterj before polluting the SeoPresentation with lots of more logic i would suggest that for your #133 work. Just injecting the typed data (SeoMetadata in your case) into it instead of letting it extract or compute it.
Maybe you can extract the SeoPresentation::getSeoMetadata() into an extra service, which extract and/or reads metadata (or one for each). Cause in my head a PresentationModel should not do that hard work - just preparing data for the view.

ElectricMaxxx · 2014-07-24T05:53:51Z

I think i am running in a circle in here. It is just an issue with implementing that optional service. But i can't really see if the issue is just a test configuration issue. When i call php app/console container:debug i see doctrine_phpcr whilst my tests fails cause it is missing.

dbu · 2014-07-24T06:11:23Z

kind of the obvious question: is phpcr-odm enabled in your test setup? and did you look at other tests in other bundles that are using the manager registry?

wouterj · 2014-07-24T08:52:08Z

You should stop using the shell file and start using the test listener stuff.

lsmith77 · 2014-07-25T06:55:31Z

ping .. we need to wrap up features soon as I want to decide what to include for CMF 1.2

wouterj · 2014-07-25T08:46:50Z

@lsmith77 SeoBundle doesn't have any new features yet (except from psr-4 autoloading). So it makes no sense to release a new minor version for this bundle.

lsmith77 · 2014-07-25T08:53:15Z

well the point is .. it would make sense if we have new features like this one :)

ElectricMaxxx · 2014-08-02T18:42:07Z

@dbu got exact that question. I think the test are cheating me. you got examples?

ElectricMaxxx · 2014-08-02T22:46:05Z

I am almost done with that. The functions i wanted to add with that PR are working fine. Just one test suite is failing: ORM. For some reason the environment = 'orm' setting does not work anymore. The complete orm setting is ignored for that test, so the manager_name/backend_type_orm isn't set right -> Exception in the MappingPass. @wouterj do you have a minute for that? Did i disable one of the configs?

ElectricMaxxx · 2014-08-20T19:32:13Z

finally i found the issue, yeaaa. Should work now.

lsmith77 · 2014-08-21T06:01:43Z

this needs a rebase now

ElectricMaxxx · 2014-08-21T06:09:30Z

done

lsmith77 · 2014-08-21T06:24:20Z

so its no longer a WIP and can be merged?

ElectricMaxxx · 2014-08-21T06:29:51Z

right, would like to squash when travis turns green.

lsmith77 · 2014-08-21T06:34:02Z

green it is :)

ElectricMaxxx · 2014-08-21T06:36:55Z

typing 10xsquash takes some time

dbu · 2014-08-21T06:39:01Z

AlternateLocaleProviderInterface.php

+     * @param array|object[]                $contents
+     * @return AlternateLocaleCollection
+     */
+    public function createForManyContent(array $contents);


kind of last moment, but why not createForContents? its just an s versus no s - but we use this naming scheme in many places and i found it convenient.

you are waking up very late to day :-) what time is it in the states now?

i only leave this saturday :-)

dbu · 2014-08-21T06:49:00Z

EventListener/ContentListener.php

@@ -11,7 +11,10 @@

 namespace Symfony\Cmf\Bundle\SeoBundle\EventListener;

+use Symfony\Cmf\Bundle\CoreBundle\Translatable\TranslatableInterface;


dbu · 2014-08-21T06:57:55Z

okay, i am through with my review. srry for only looking at it now. i think its coming good, just a few details, mostly code cleanup

ElectricMaxxx · 2014-08-21T06:58:46Z

no problem, everything sane you said.

ElectricMaxxx · 2014-08-21T07:11:12Z

@lsmith77 i see a lot of green now, bu now you can't merge anymore.

lsmith77 · 2014-08-21T07:29:52Z

yeah .. just ping me when its ready

ElectricMaxxx · 2014-08-21T09:04:33Z

Except the issue i don't understand i would call it ready.

ElectricMaxxx · 2014-08-21T09:52:53Z

DependencyInjection/CmfSeoExtension.php

+        if (null !== $alternateLocaleProviderDefinition) {
+            $definition = $container->getDefinition('cmf_seo.event_listener.seo_content');
+            $definition
+                ->addMethodCall('setAlternateLocaleProvider', array($alternateLocaleProviderDefinition));


I think i got it as you both (@dbu, @lsmith77 ) wanted it.
Either an user sets its own provider under the served parameter with an service id (won't check that, he needs to feel the pain of an error), or, if defined, we use the custom one with the PHPCR behavior.

almost. if you create this defaultAlternateLocaleProvider you can do

$alternateLocaleProvider = empty($config['alternate_locale']['provider_id']) ? $this->defaultAlternateLocaleProvider : $config['alternate_locale.provider_id']; if ($alternateLocaleProvider) { $definition = $container->getDefinition('cmf_seo.event_listener.seo_content'); $definition ->addMethodCall('setAlternateLocaleProvider', array($this->getDefinition($alternateLocaleProvider))) ; }

no need to again check if the container knows the service. we do want an error if $config['alternate_locale']['provider_id'] does not exist so we know.

ElectricMaxxx · 2014-08-21T11:48:55Z

@lsmith77 that should be fine now. Right @dbu?

dbu · 2014-08-21T12:17:06Z

Doctrine/Phpcr/AlternateLocaleProvider.php

+     *
+     * @param array|object[] $contents
+     *
+     * @return AlternateLocaleCollection


this returns an AlternateLocaleCollection[], right? a list of alternate locale collections.

dbu · 2014-08-21T12:19:15Z

yeah, coming good.

dbu · 2014-08-21T12:29:08Z

i just squashed all commits and force pushed. i think its good now. waiting for a last travis green light, then lets merge!

ElectricMaxxx · 2014-08-21T12:30:24Z

ah that was you, got that window open and thought: Wtw?!? Saw the move and the new commit with my avatar... :-)

dbu · 2014-08-21T12:31:24Z

the powers of git :-P

implementing alternate languages for content documents

ElectricMaxxx reviewed Jul 11, 2014
View reviewed changes

ElectricMaxxx mentioned this pull request Jul 12, 2014

Create XML Sitemap #132

Closed

dbu reviewed Jul 14, 2014
View reviewed changes

ElectricMaxxx reviewed Jul 22, 2014
View reviewed changes

ElectricMaxxx force-pushed the lang-alternate branch from 93fb005 to d326bc7 Compare August 21, 2014 06:08

ElectricMaxxx changed the title ~~[WIP] implementing alternate languages for content documents~~ implementing alternate languages for content documents Aug 21, 2014

ElectricMaxxx force-pushed the lang-alternate branch from d326bc7 to ec65e44 Compare August 21, 2014 06:36

dbu reviewed Aug 21, 2014
View reviewed changes

ElectricMaxxx reviewed Aug 21, 2014
View reviewed changes

dbu reviewed Aug 21, 2014
View reviewed changes

implementing alternate languages for content documents

1000a69

dbu force-pushed the lang-alternate branch from 23e6506 to 1000a69 Compare August 21, 2014 12:28

lsmith77 added a commit that referenced this pull request Aug 21, 2014

Merge pull request #175 from symfony-cmf/lang-alternate

77ea44b

implementing alternate languages for content documents

lsmith77 merged commit 77ea44b into master Aug 21, 2014

lsmith77 deleted the lang-alternate branch August 21, 2014 13:09

		@@ -11,7 +11,10 @@

		namespace Symfony\Cmf\Bundle\SeoBundle\EventListener;

		use Symfony\Cmf\Bundle\CoreBundle\Translatable\TranslatableInterface;

implementing alternate languages for content documents #175

implementing alternate languages for content documents #175

Conversation

ElectricMaxxx commented Jul 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElectricMaxxx commented Jul 12, 2014

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

ElectricMaxxx commented Jul 17, 2014

dbu commented Jul 21, 2014

Choose a reason for hiding this comment

ElectricMaxxx commented Jul 24, 2014

dbu commented Jul 24, 2014

wouterj commented Jul 24, 2014

lsmith77 commented Jul 25, 2014

wouterj commented Jul 25, 2014

lsmith77 commented Jul 25, 2014

ElectricMaxxx commented Aug 2, 2014

ElectricMaxxx commented Aug 2, 2014

ElectricMaxxx commented Aug 20, 2014

lsmith77 commented Aug 21, 2014

ElectricMaxxx commented Aug 21, 2014

lsmith77 commented Aug 21, 2014

ElectricMaxxx commented Aug 21, 2014

lsmith77 commented Aug 21, 2014

ElectricMaxxx commented Aug 21, 2014

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

dbu commented Aug 21, 2014

ElectricMaxxx commented Aug 21, 2014

ElectricMaxxx commented Aug 21, 2014

lsmith77 commented Aug 21, 2014

ElectricMaxxx commented Aug 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElectricMaxxx commented Aug 21, 2014

Choose a reason for hiding this comment

dbu commented Aug 21, 2014

dbu commented Aug 21, 2014

ElectricMaxxx commented Aug 21, 2014

dbu commented Aug 21, 2014