EZP-24800 Support allowed classes limitation for object relation fields #2022

slaci · 2017-06-18T09:44:27Z

This PR adds allowed classes to single object relation field. @andrerom suggested this PR, which does the same for legacy:
ezsystems/ezpublish-legacy#1201

This feature requires changes in 3 bundles, so I have 3 PRs. The other 2:

EZP-24800 Support allowed classes limitation for object relation fields PlatformUIBundle#878
EZP-24800 Support allowed classes limitation for object relation fields repository-forms#127

I made the storage of the settings in xml in the dataText5 field. It is compatible with the old way, but it drops that schema on the next save of the content type. So if someone has stuff in dataInt1 and dataInt2 cause of an upgrade, then those will work, but when someone saves the content type, then they will be emptied and converted to xml into dataText5.

I didnt include the type and object_class deprecated xml tags in this xml.

I ran test with this command: bin/phpunit vendor/ezsystems/ezpublish-kernel/ --testsuite "eZ\Publish\Core\FieldType" --filter "RelationTest"

andrerom · 2017-06-18T17:49:29Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/fielddefinition_settings.html.twig

+        <div class="ez-fielddefinition-setting-name">{{ 'fielddefinition.allowed-content-types.label'|trans|desc("Allowed content types:")}}</div>
+        <div class="ez-fielddefinition-setting-value">
+            {% if settings.selectionContentTypes %}
+                {# TODO display content type name #}


note to self; ideally we need a multi get api end point where you can get content types by ~~identifiers~~ id*, then a twig function or otherwise can get them easily as a followup to this. Role editing has same need and more.

*as stated later below this should actually have been by id to avoid risk of being out of date on content type identifier changes

Yes that would be very useful (not just here, everywhere). I just copied the content to this new block without changes anyway.

andrerom

Looks great, what I'm missing is integration test coverage for the added feature. However besides inline comment did not see further examples of that when checking quickly. Maybe @alongosz knows.

andrerom · 2017-06-18T17:53:26Z

eZ/Publish/API/Repository/Tests/FieldType/RelationIntegrationTest.php

@@ -109,7 +113,7 @@ public function getValidatorSchema()
     */
    public function getValidFieldSettings()
    {
-        return array('selectionMethod' => 0, 'selectionRoot' => '1');
+        return array('selectionMethod' => 0, 'selectionRoot' => '1', 'selectionContentTypes' => array());


Like in RelationListIntegrationTest.php this can have a valid value.

And here and other places where you add arrays, use PHP 5.4+ format [].

And here and other places where you add arrays, use PHP 5.4+ format [].

I used the old syntax cause it is used everywhere in the file, well every file I touched for these changes... didnt want to mix the style. If I fix in the whole file then the diff becomes messy, if I fix only for added lines then the source will be ugly I think.

I'm missing is integration test coverage for the added feature

I don't know how could I test this. On PHP level there is no validation for this feature. Only the js constraints the selectable content types. I guess the validate() method could check if the selected content is really in an expected content type, but relation list does'nt do this either.
On the js side, its the universal discovery's feature to constraint the stuff, so that should have the tests for that part, not this field.

I used the old syntax cause it is used everywhere in the file, well every file I touched for these changes... didnt want to mix the style. If I fix in the whole file then the diff becomes messy, if I fix only for added lines then the source will be ugly I think.

Well, we need our code to evolve (a.k.a not making a revolution), so we decided that for modified and new code we'll use short array syntax. So if we modified a big multidimensional array, we'd change the whole thing. Could be as a separate commit. However we don't change unrelated and unmodified blocks so diff won't get too messy. Look at this as a compromise :)

andrerom · 2017-06-19T07:51:25Z

eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/RelationConverter.php

+        if (!empty($fieldSettings['selectionContentTypes'])) {
+            foreach ($fieldSettings['selectionContentTypes'] as $typeIdentifier) {
+                $allowedClass = $doc->createElement('allowed-class');
+                $allowedClass->setAttribute('contentclass-identifier', $typeIdentifier);


Another note to self: This is actually a bit of a problem. We don't guarantee identifier to stay constant over time. And have no way or intention to have to rewrite data for all fields or field definitions on such changes. So ideally we should store ID here, and as before expose identifer to user in API. But I guess that would need to be in addition to identifier in storage format for BC reasons, and I'm aware this is mistake from the past and not yours :)

The transition should be easy as I did with the other fields (dataInt1 and dataInt2). We just need a new tag for the XML. Set the old tag's data as default if present (and delete it), and overwrite with the new tag's content if thats there. So later all xml's would only have the new tag.

The converter is a service so it should be easy to inject the content type service into it to resolve the ids. Just so many queries... content type service really should cache 😆 Or that multiple endpoint would be useful there.

alongosz

Looks great, what I'm missing is integration test coverage for the added feature.

What could be done for integration tests, besides checking for valid/invalid field settings (which is already done) is to create Relation field limited to e.g. blog_post and try to create relation using different content type to check if it throws exception (assuming I understood how it should work :-))

New feature exposed in Twig template should be tested using Behat on repository-forms, but AFAIK we don't have scenarios for content type at all (or do we...?)

Besides, one small PhpDoc nitpick below:

alongosz · 2017-06-19T09:21:21Z

eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/RelationConverter.php

+     *   <constraints/>
+     *   <selection_type value="0"/>
+     *   <contentobject-placement/>
+     * </related-objects>


Please wrap the whole code example (starting with the first <?xml) in <code></code> tags so it looks good on doc preview in IDE (at least in PhpStorm) :)

Ok I added the <code></code>. I added it for RelationList too, thats where I copied the original stuff from anyway.

is to create Relation field limited to e.g. blog_post and try to create relation using different content type to check if it throws exception

It won't throw anything. On PHP level it creates anything happily. But this is the RelationList's behavior too. The setting is only affecting the JS validation atm.

andrerom

From my side +1, the additional integration tests is missing for both fields so that is maybe for us to figure out and my comments on usage of identifier is follow mistake from relation list so we can deal with that later as separate effort.

alongosz

ok, +1 then :)

andrerom · 2017-07-18T08:59:34Z

Status: Waiting on some additional tests on PlatformUI Pr which should be merged at the same time.

bdunogier

I like the idea, it's a cool contribution.

I'd like the settings / XML serialization process to be made more readable. Once we have figured that out, you have my approval :)

bdunogier · 2017-07-20T08:15:02Z

eZ/Publish/Core/FieldType/Relation/Type.php

+                        $validationErrors[] = new ValidationError(
+                            "Setting '%setting%' value must be of array type",
+                            null,
+                            [


Nitpick: I would put the whole array on one line.

bdunogier · 2017-07-20T08:18:10Z

eZ/Publish/Core/Persistence/Legacy/Content/FieldValue/Converter/RelationConverter.php

-        $storageDef->dataInt2 = (int)$fieldDef->fieldTypeConstraints->fieldSettings['selectionRoot'];
+        $constraints = $doc->createElement('constraints');
+        if (!empty($fieldSettings['selectionContentTypes'])) {
+            foreach ($fieldSettings['selectionContentTypes'] as $typeIdentifier) {


The mapping of settings to XML, and vice-versa below, is a bit long for procedural code within that method.

It would be more readable if moved to a private class method. The option above is to move it to a dedicated service/class. Doing so would ease unit testing.

@bdunogier Are you suggesting we should do something like $storageDef->dataText5 = $this->getXMLForStorageFieldDefinition($fieldDef->fieldTypeConstraints->fieldSettings);, or something else?

Implied here is that he will need to adopt RelationListConverter as well, part of the point here is to keep them in sync. Could perhaps also extract some of the logic to RelationConverterTrait and reuse, wdyt @slaci ?

I was suggesting something like:

public function toStorageFieldDefinition(FieldDefinition $fieldDef, StorageFieldDefinition $storageDef) { // [...] $root->appendChild($this->mapConstraintsToDOM($doc, $fieldSettings, )); $root->appendChild($this->mapSelectionTypeToDOM($doc, $fieldSettings)); $root->appendChild($this->mapDefaultSelectionToDOM($doc, $fieldSettings)); } private function mapConstraintsToDOM(DOMDocument $doc, array $fieldSettings) { $constraints = $doc->createElement('constraints'); if (!empty($fieldSettings['selectionContentTypes'])) { foreach ($fieldSettings['selectionContentTypes'] as $typeIdentifier) { $allowedClass = $doc->createElement('allowed-class'); $allowedClass->setAttribute('contentclass-identifier', $typeIdentifier); $constraints->appendChild($allowedClass); unset($allowedClass); } } return $constraints; } private function mapSelectionTypeToDOM(DOMDocument $doc, array $fieldSettings) { $selectionType = $doc->createElement('selection_type'); if (isset($fieldSettings['selectionMethod'])) { $selectionType->setAttribute('value', (int)$fieldSettings['selectionMethod']); } else { $selectionType->setAttribute('value', 0); } return $selectionType; } private function mapDefaultSelectionToDOM(DOMDocument $doc, array $fieldSettings) { $defaultLocation = $doc->createElement('contentobject-placement'); if (!empty($fieldSettings['selectionRoot'])) { $defaultLocation->setAttribute('node-id', (int)$fieldSettings['selectionRoot']); } return $defaultLocation; }

And the mirror methods for DOM -> storage.
But it's not a hard requirement.

andrerom · 2017-07-31T12:24:53Z

We'll see if we can handle the code xml cleanup across existing field type and the new code in followup PR, merging for 1.11, thanks @slaci!

…ds (ezsystems#2022) * EZP-24800 Support allowed classes limitation for object relation fields * EZP-24800 code style fixes

This was referenced Jun 18, 2017

EZP-24800 Support allowed classes limitation for object relation fields ezsystems/PlatformUIBundle#878

Merged

EZP-24800 Support allowed classes limitation for object relation fields ezsystems/repository-forms#127

Merged

andrerom reviewed Jun 18, 2017

View reviewed changes

andrerom suggested changes Jun 18, 2017

View reviewed changes

EZP-24800 Support allowed classes limitation for object relation fields

4f5ebe8

andrerom reviewed Jun 19, 2017

View reviewed changes

alongosz requested changes Jun 19, 2017

View reviewed changes

EZP-24800 code style fixes

87e62f8

andrerom approved these changes Jun 22, 2017

View reviewed changes

alongosz approved these changes Jun 23, 2017

View reviewed changes

bdunogier requested changes Jul 20, 2017

View reviewed changes

andrerom merged commit dded74a into ezsystems:master Jul 31, 2017

andrerom mentioned this pull request Oct 20, 2017

EZP-28118: Error in RelationConverter on content after editing class in legacy #2115

Merged

1 task

masacc mentioned this pull request Dec 14, 2017

EZP-24800 Support allowed classes limitation for object relation fields #2191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-24800 Support allowed classes limitation for object relation fields #2022

EZP-24800 Support allowed classes limitation for object relation fields #2022

slaci commented Jun 18, 2017 •

edited

andrerom Jun 18, 2017 •

edited

slaci Jun 19, 2017

andrerom left a comment

andrerom Jun 18, 2017

slaci Jun 19, 2017 •

edited

alongosz Jun 19, 2017

andrerom Jun 19, 2017

slaci Jun 20, 2017

alongosz left a comment

alongosz Jun 19, 2017

slaci Jun 20, 2017

slaci Jun 20, 2017

andrerom left a comment

alongosz left a comment

andrerom commented Jul 18, 2017

bdunogier left a comment

bdunogier Jul 20, 2017

bdunogier Jul 20, 2017

andrerom Jul 26, 2017

bdunogier Jul 27, 2017

andrerom commented Jul 31, 2017 •

edited

EZP-24800 Support allowed classes limitation for object relation fields #2022

EZP-24800 Support allowed classes limitation for object relation fields #2022

Conversation

slaci commented Jun 18, 2017 • edited

andrerom Jun 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slaci Jun 19, 2017 • edited

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

alongosz left a 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

andrerom left a comment

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

andrerom commented Jul 18, 2017

bdunogier left a 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

andrerom commented Jul 31, 2017 • edited

slaci commented Jun 18, 2017 •

edited

andrerom Jun 18, 2017 •

edited

slaci Jun 19, 2017 •

edited

andrerom commented Jul 31, 2017 •

edited