menu label should not be nullable

[dbu]

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

with this change, the sandbox menu behaves correct again. i notice that somewhere we use the phpcr node name as fallback if the label is not defined. but i think it makes more sense to have the label required.

the underlying issue is that a while ago, we broke the attribute translation strategy for nullable fields when a translation locale does not exist at all, as all fields then become null:
https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/AttributeTranslationStrategy.php#L94

i will try to find a solution for phpcr-odm, but its a very tricky problem.

[dbu]

ok this is a phpcr-odm bug about translations. i am cleaning this stuff up but there is a couple of surprises in there. will need to wrap this up tomorrow, sorry.

[lsmith77]

any clue when we broke this?

[lsmith77]

seems like it works fine on cmf.liip.ch .. so it must have been introduced since the last tagging session.

[lsmith77]

So this is the current state:

[18:29:20][lsmith@Soitgoes cmf-sandbox](prepare-rc1 $)$ app/console doctrine:phpcr:node:dump /cms/menu/main/demo-item/controller-item --depth=0 --props
controller-item:
  - menuContent =
   - : d373824a-367e-4bfb-ae89-ff5cd2b816ea
  - childrenAttributes = Array()
  - routeParameters = Array()
  - display = 1
  - jcr:mixinTypes = Array(    [0] => phpcr:managed)
  - labelAttributes = Array()
  - routeAbsolute =
  - displayChildren = 1
  - jcr:primaryType = nt:unstructured
  - linkAttributes = Array()
  - phpcr:class = Symfony\Cmf\Bundle\MenuBundle\Doctrine\Phpcr\MenuNode
  - phpcr:classparents = Array(    [0] => Symfony\Cmf\Bundle\MenuBundle\Model\MenuNodeBase    [1] => Symfony\Cmf\Bundle\MenuBundle\Model\MenuN...
  - phpcr_locale:en-label = Explicit controller
  - publishable = 1
  - phpcr_locale:ennullfields = Array(    [0] => uri)
  - attributes = Array()

This is the sandbox beta1 state:

[18:27:40][lsmith@Soitgoes cmf-sandbox_beta2]((1.0.0-beta1))$ app/console doctrine:phpcr:node:dump /cms/menu/main/demo-item/controller-item --depth=0 --props
controller-item:
  - weakContent =
   - : 78077a1a-69d0-4faf-8bfe-15df61678496
  - childrenAttributes = Array()
  - phpcr:classparents = Array()
  - phpcr:class = Symfony\Cmf\Bundle\MenuBundle\Document\MenuNode
  - routeParameters = Array()
  - label = Explicit controller
  - isPublishable = 1
  - attributes = Array()
  - weak = 1

So there is no regression, the issue is simply that we are now using a multilingual document in all cases. As such yes it seems making the field nullable triggers a special case in which we will then continue looking for another translation.

[lsmith77]

This is the exception I am talking about:
https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/AttributeTranslationStrategy.php#L94

Now the question if the field is not nullable and we did not store it as a nullable field, why do we not just continue rather than setting the default values? Is this to be more graceful when a new property is added? If that is show we might need to do 2 passes, one were we do not set a default null/array() value if a non nullable non nullfield is missing first and then second again with the current behavior.

[lsmith77]

In other words things work fine if I do

diff --git a/lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/AttributeTranslationStrategy.php b/lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/AttributeTranslationStrategy.php
index a23944a..2c507ea 100644
--- a/lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/AttributeTranslationStrategy.php
+++ b/lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/AttributeTranslationStrategy.php
@@ -90,11 +90,7 @@ class AttributeTranslationStrategy extends AbstractTranslationStrategy
                     }
                 }
             } else {
-                // Could not find the translation in the given language
-                if (!$metadata->mappings[$field]['nullable']) {
-                    return false;
-                }
-                $value = (true === $metadata->mappings[$field]['multivalue']) ? array() : null;
+                return false;
             }
             $metadata->reflFields[$field]->setValue($document, $value);
         }

[lsmith77]

see doctrine/phpcr-odm#303