[WIP] Persistence #55

dantleech · 2013-08-05T16:15:21Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	yes
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	symfony-cmf/symfony-cmf-docs#233

Implementing Model model ... FOR REVIEW

dantleech · 2013-08-05T16:17:29Z

@dbu thinks that we should not support the *Base model for this bundle, and I tend to agree.

Another question is if we want to support other persistence layers here? I..e do I need to create cmf_menu.persistence.phpcr.route_provider parameter and such like it?

dbu · 2013-08-05T16:20:26Z

i am very much +1 for allowing other persistence layers. this is
actually simpler to support in orm than the routing + content things.

dbu · 2013-08-05T17:20:41Z

CmfSimpleCmsBundle.php

+                        realpath(__DIR__ . '/Resources/config/doctrine-model') => 'Symfony\Cmf\Bundle\SimpleCmsBundle\Model',
+                        realpath(__DIR__ . '/Resources/config/doctrine-phpcr') => 'Symfony\Cmf\Bundle\SimpleCmsBundle\Doctrine\Phpcr',
+                    ),
+                    array('cmf_simple_cms.manager_name')


lets make this cmf_simple_cms.persistence.phpcr.manager_name to be consistent

dantleech · 2013-08-06T11:15:52Z

This is turning into a bit of an epic refactor - there sure is alot of DI code.

As we are removing Multilang and bundling that functionality by default, is there anything else we can do to make the SimpleCMS bundle more simple, especially in terms of the custom Routing setup? // cc @lsmith77

dbu · 2013-08-06T17:53:06Z

i propose to finalize what is in this PR and do further things in a new PR after this one is merged.

i think the main thing with the routes is about multilang, as per RoutingBundle the locale is part of the repository path, but for simplecms a special provider is used to extract the locale in the lookup (as we only have one route that is not translated). i guess we should keep that functionality, though maybe it could be simplified. right now we clone the dynamic router service definition and add all kind of things. not sure if there could be something simpler or more generic.

we should not just replace/reconfigure the normal dynamic router with this one, or the simplecms bundle could not be combined with things using routing bundle directly...

lsmith77 · 2013-08-06T18:55:53Z

agreed

dantleech · 2013-08-07T10:13:30Z

DataFixtures/AbstractLoadPageData.php

@@ -12,7 +12,7 @@

 use Symfony\Cmf\Component\Routing\RouteObjectInterface;

-abstract class LoadCmsData extends ContainerAware implements FixtureInterface, OrderedFixtureInterface
+abstract class AbstractLoadPageData extends ContainerAware implements FixtureInterface, OrderedFixtureInterface


This seemed like a more appropriate name.

dantleech · 2013-08-07T10:26:06Z

ok, this now seems to work with Multilang. But it doesn't work with non-multilang.

e.g. the test fixtures create "/homepage", but this will only work with "/en/homepage".

The bundle doesn't work if we ommit multilang.locales.

I think we need someway to not add the static prefix /{locale} if locales are not set -- or even better if we can determine if the document has any translations or not. ??

lsmith77 · 2013-08-07T13:15:12Z

To be able to determine if the document has any translations we would need access to the DocumentManager. I guess what we could do is set something on the Route to enable multi lang for the given instance. I see 2 ways:

we do this dynamically inside the RouteProvider
we do this "semi" dynamically by modifying the state via a listener and then persisting a flag

dantleech · 2013-08-07T15:04:46Z

Resources/config/routing.xml

        <parameter key="cmf_simple_cms.generator_class">Symfony\Cmf\Bundle\RoutingBundle\Routing\ContentAwareGenerator</parameter>
-        <parameter key="cmf_simple_cms.phpcrodm_route_idprefix_listener_class">Symfony\Cmf\Bundle\RoutingBundle\Doctrine\Phpcr\IdPrefixListener</parameter>
        <parameter key="cmf_simple_cms.enhancer_route_content_class">Symfony\Cmf\Component\Routing\Enhancer\RouteContentEnhancer</parameter>
        <parameter key="cmf_simple_cms.enhancer_explicit_template_class">Symfony\Cmf\Component\Routing\Enhancer\FieldPresenceEnhancer</parameter>
        <parameter key="cmf_simple_cms.enhancer_controllers_by_alias_class">Symfony\Cmf\Component\Routing\Enhancer\FieldMapEnhancer</parameter>


@dbu these enhancer class parameters do not seem to be used anywhere.

if we have our own router, we also need our own enhancers i think.

I'm not really sure about the implications of what we currently do (using the RoutingBundle enhancers). But in anycase, these parameters are not used anywhere.

Are you suggesting that we create these enhancer services?

if we don't configure enhancers in the bundle config and everything works, lets drop them and see what happens :-) if we did not need them until now, lets drop them. not sure how the main content goes into the request without any enhancer though.

The enhancers seem to be implemented in the DI Extension with DefinitionDecorator classes, which are a bit foreign to me.

ah right. but guess we need them and can't drop them then. maybe add a
to those parameters then?
i think the decorator thing is to avoid instantiating the simple router
if its not used ever. we can't just tag the enhancers as the tag adds
them to the default dynamic router only. not sure if that could be
expressed in xml as well, if its static stuff.

dantleech · 2013-08-08T12:38:49Z

ok, this is potentially done as far as I'm concerned, so good to review.

For the multilang stuff I have simply exposed the setAddLocalePattern method - I tried the alternative (only adding the locale if the document is translated) but this didn't work as all documents are "translated" and counting the number of translations seemed wrong (what if you delete a translation, should the URL for the page change just because there is only one left?).

lsmith77 · 2013-08-08T12:42:16Z

uhm .. the getLocales() method in the ODM only counts actual translations done via bindTranslation afaik. if not then maybe we should make it so. once things are like that then i would enable multilang behavior even if there is only one translation.

dantleech · 2013-08-08T12:47:17Z

DataFixtures/Phpcr/AbstractLoadPageData.php

@@ -12,10 +12,9 @@

 use Symfony\Cmf\Component\Routing\RouteObjectInterface;

-abstract class LoadCmsData extends ContainerAware implements FixtureInterface, OrderedFixtureInterface
+abstract class AbstractLoadPageData extends ContainerAware implements FixtureInterface, OrderedFixtureInterface


I wonder if we shouldn't simply remove this class. Its a bit of a convenience hack, there are probably other libraries that do this sort of thing much better, and it isn't /that/ much harder to do this ad-hoc in the concrete class.

well it makes it very easy to get started. if we remove it we have to add it to the SE for example https://github.com/symfony-cmf/symfony-cmf-standard/blob/master/src/Acme/MainBundle/DataFixtures/PHPCR/LoadSimpleCmsData.php

however we could consider basing this on https://github.com/nelmio/alice

That looks like a really cool library, +1 for adopting it in the SE / Sandbox and dropping this, if possible.

fine for me

dbu · 2013-08-08T12:51:07Z

@lsmith77 you mean getLocalesFor? yes, that is only the locales of the current document. there is a method getDefaultLocalesOrder which should return all locales (though currently its missing to return the first locale)

or you could keep configuring the locales into this bundle if we need them.

dbu · 2013-08-08T12:54:15Z

the corebundle un-translate listener sets the locale to false if it removes translations. you could check getLocale() === false

dbu · 2013-08-08T12:55:39Z

Model/Page.php

+
+        $prefix = $this->getPrefix();
+        $path = substr(parent::getId(), strlen($prefix));
+        $path = $prefix.'/{_locale}'.$path;


should this not depend on the addLocalePattern flag?

We have already returned by now if that flag is set.

sorry, was not reading carefully

dantleech · 2013-08-10T09:53:44Z

Regarding the "automatic" prefixing of the locale in the URL depending on if it has bound translations or not - its not clear to me on how to proceed:

If we prefix the locale automatically, then the action of translating a document is irreversable, the user cannot remove the bound translation - even if done by accident in the admin interface.

What happens in this PR currently is this:

The user calls setAddLocalePattern on the document or they set this flag in the admin interface.

So the user chooses what happens, much like they would have choosen if they were going to use MultilangPage or Page, so, less magic.

And I wonder if there isn't some scope here for a new Translatable interface, e.g. MutableTranslatableStateInterface (why isn't there a neat word for enable/disable-able?) - which would allow us to explicitly enable / disable translation on a document via. a flag.

EDIT: Found one: http://en.wiktionary.org/wiki/activable#English :)

lsmith77 · 2013-08-11T18:23:35Z

CHANGELOG.md

+* **2013-08-10**: Removed "tags" property, see: https://github.com/symfony-cmf/SimpleCmsBundle/issues/53
+* **2013-08-08**:
+  * Seperate Multilang document now incorporated in single document
+  * `multilang.locales` config removed (use cmf_core.multilang.locales)


why? I prefer each Bundle to have its own setting and rely on CoreBundle to ease handling for duplicate settings.

does SimpleCmsBundle itself still do anything with locales? if not, there is no need to configure them. but maybe the routes with the automatic locale prefixing need it to put a requirement on _locale?

lsmith77 · 2013-08-12T06:13:12Z

So I guess what we need now is:

a rebase for this PR
a migration script for the old document
an integration with Alice in the SE + sandbox

- Multilang works, non-multilang doesnt

- Collapsed PageBase into Page

dbu · 2013-08-14T10:14:09Z

lets keep the old way and do alice later, so that we can merge this PR soon

dbu · 2013-08-14T10:14:45Z

Model/Page.php


    /**
     * Extra values an application can store along with a page
-     *
-     * @PHPCRODM\String(assoc="")
     */
    protected $extras;


we should init this with = array()

dantleech · 2013-08-14T10:25:49Z

Migration instructions in CHANGELOG.md, they depend on https://github.com/doctrine/DoctrinePHPCRBundle/pull/84/files

dantleech · 2013-08-14T10:55:00Z

ok, I have added a small test for the migrator and re-added DataFixtures base-class.

This is potentially good to merge I think.

[WIP] Persistence

lsmith77 · 2013-08-19T06:03:08Z

looks like the tests need fixes for the non nullable fields

lsmith77 · 2013-08-19T06:24:55Z

ah .. is this fixed by changes in RoutingBundle?

dbu reviewed Aug 5, 2013
View reviewed changes

dantleech reviewed Aug 7, 2013
View reviewed changes

dantleech reviewed Aug 8, 2013
View reviewed changes

dbu reviewed Aug 8, 2013
View reviewed changes

This was referenced Aug 10, 2013

Multilang page admin #51

Closed

Remove "tags" from Page #53

Closed

lsmith77 reviewed Aug 11, 2013
View reviewed changes

dantleech added 12 commits August 14, 2013 09:05

Moved models to Model model

e77a72b

Hmm.. do we want to support other persistence layers?

c10ff18

Further refactorings

4341d5a

Added test application basics

1ff94e1

- Multilang works, non-multilang doesnt

Made Page aware of its translatanle state

bbd5948

Fixed multilang issues, added tests

4c47714

Tidying up

b60b097

Moved $node to Phpcr Doc

3e733a2

Removed "tags" property and ...

6361d56

- Collapsed PageBase into Page

Removed PageBase

c144f85

Fixed test

7361c29

Rebased and removed Base data fixtures class

a9b85f5

dbu reviewed Aug 14, 2013
View reviewed changes

Added migration instructions

aa6c6d7

dantleech added 3 commits August 14, 2013 11:34

Code review fixes

cb4f98c

Added migrator test

4eb849a

Readded data fixtures class

c6d8498

dantleech added 2 commits August 14, 2013 18:04

Re-added DataFixtures base class

2610d69

Updated for latest CMF changes

839945d

lsmith77 added a commit that referenced this pull request Aug 14, 2013

Merge pull request #55 from symfony-cmf/persistence

de0142c

[WIP] Persistence

lsmith77 merged commit de0142c into master Aug 14, 2013

lsmith77 deleted the persistence branch August 14, 2013 19:43

[WIP] Persistence #55

[WIP] Persistence #55

Uh oh!

Conversation

dantleech commented Aug 5, 2013

Uh oh!

dantleech commented Aug 5, 2013

Uh oh!

dbu commented Aug 5, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Aug 6, 2013

Uh oh!

dbu commented Aug 6, 2013

Uh oh!

lsmith77 commented Aug 6, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Aug 7, 2013

Uh oh!

lsmith77 commented Aug 7, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Aug 8, 2013

Uh oh!

lsmith77 commented Aug 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Aug 8, 2013

Uh oh!

dbu commented Aug 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Aug 10, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsmith77 commented Aug 12, 2013

Uh oh!

dbu commented Aug 14, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Aug 14, 2013

Uh oh!

dantleech commented Aug 14, 2013

Uh oh!