Support child restriction by dantleech · Pull Request #706 · doctrine/phpcr-odm

dantleech · 2016-05-30T09:32:35Z

TODO

Wrap up feature
Documentation (also mention that if a document already is there because the situation was created before the annotation was used or from raw phpcr operations, the child will be on the document as validation only happens during save but not on read)

Doc PR: doctrine/phpcr-odm-documentation#81

This PR allows the restriction of child document types:

/**
 * @PHPCRODM\Document(restrictChildren={
 *   "Doctrine\Tests\Models\CMS\CmsArticle"
 * })
 */
 class ArticleFolder
 {
     // ...
 }

By default children are unrestricted (childRestrictions = null)
Children validated on "insert".
Children validated on "move".

Why?

Ensure structural integrity where it matters.
Determine which types can be added to a document (e.g. in a tree browser).
Ultimately this could also be part of the node type definition if we ever implementing that.

Alternatives

We could also implement this as a separate annotation:

/**
 * @Document()
 * @RestrictChildren({
 *   "AppBundle\Document\Foo"
 *   "AppBundle\Document\Bar"
 * }, restrict="include")
 */

Where restrict is an enum of none, include and exclude.

dantleech · 2016-05-30T09:33:02Z

-        if ($node->hasProperty('phpcr:class')) {
-            $nodeClassName = $node->getProperty('phpcr:class')->getString();
+        if ($node->hasProperty(self::CLASS_PROPERTY)) {
+            $nodeClassName = $node->getProperty(self::CLASS_PROPERTY)->getString();


Grep to see if phpcr:class is referenced anywhere else.

Actually this is too risky (adding class constants to places that might be untested) and unrequired for this PR, reverting.

agreed. but its a good idea to do, maybe in a separate PR once this one is merged?

dantleech · 2016-05-30T09:35:54Z

Would need to add XML and YAML drivers.

dbu · 2016-05-31T12:50:44Z

interesting proposal! indeed the outside tree definition we currently have is awkward and weak. and i always forget to add my new classes to it.

i wonder if we should do this on the class level or on Child / Children mappings and then merge all Child / Children mappings to know the allowed children. or maybe keep what you propose, but add that when the annotation / attribute is present the child and children types are merged into the allowed list automatically. then i can potentially limit my model by having children explicitly mapped and just set an empty annotation / attribute to trigger restriction. i guess restricting by default is not that user friendly (and a huge BC break that can be annoying even when upgrading to 2.0 at some point)

i think i prefer the separate annotation, given that we could have options. it could be something like

/**
 * @Document()
 * @RestrictChildren(
 *    include={"AppBundle\Model\MarkerInterface"}, 
 *    exclude={"AppBundle\Document\SpecialCase"}
 * )
 */

dantleech · 2016-05-31T17:10:55Z

i wonder if we should do this on the class level or on Child / Children mappings and then merge all Child / Children mappings to know the allowed children

I did think about doing it on the Children mapping - but that applies to a property (i.e. there are many properties even if you can only map one as having the Children) - you might not even want to map the children to a property.

I think its correct to say "this document can have these children" rather than "this document which has a property mapped as children wiith these types can have those children".

i guess restricting by default is not that user friendly (and a huge BC break that can be annoying even when upgrading to 2.0 at some point)

If we add an extra annotation then I suggest that the Metadata has an enum $childRestriction with the values none, inclusive and exclusive, with the default being none.

So the annotation would be fully expressed:

 * @RestrictChildren(
 *    classes={"AppBundle\Model\MarkerInterface"}, 
 *    type="include"
 * )

If classes are present then we could automatically switch from none to include.

dbu · 2016-06-01T08:00:16Z

i prefer to have the explicit lists (see the example i created) as it allows you to allow everything of a base class but exclude some exceptions. like the sonata admin extensions do. an empty @RestrictChildren would say there can be no children.

and i would merge types from Child / Children mappings into the allowed children. otherwise you need to duplicate that. there can be several Children annotations, btw. you can restrict them to prefixes and use a naming pattern to separate children. avoids you to have Child with a container object.

dantleech · 2016-06-01T15:45:22Z

I prefer to have the explicit lists (see the example i created) as it allows you to allow everything of a base class but exclude some exceptions.

ok thats makes sense.

and i would merge types from Child / Children mappings into the allowed children. otherwise you need to duplicate that.

I don't think you can set type for Child / Children? only a node-name filter?

dbu · 2016-06-02T06:22:56Z

I don't think you can set type for Child / Children? only a node-name filter?

ups, you are right. i guess the feature would make sense, but we don't have to mix it into this PR. maybe we could have the minimal sanity check that when you forbid all children but have a Child / Children mapping, we raise an error that this makes no sense?

dbu · 2016-06-02T06:24:31Z

+     * READ-ONLY: Child class restrictions.
+     *
+     * @var array
+     */


child or children? not sure which is the correct way. its the restriction on each child...

if we were to follow from PHPCR we could just have requiredClasses as with NodeDefinition::getRequiredPrimaryTypes

Also interesting that they do not permit blacklisting (afaict)

But maybe this is an argument to keep it as a whitelist - as if we do allow mapping by phpcr_type in the future, this feature would not map to it.

actually yes, lets only use whitelisting then. finer grained stuff can be done with listeners or custom validators and whatnot. and go with requiredClasses - i like to keep this as close to PHPCR as makes sense. but i think PHPCR also does the equivalent of an instanceof check, so we want to keep doing that.

Shall we go with a requiredClasses attribute or a separate annotation?

attribute is better imho, now that we decided it can be simply a list of allowed classes

dbu · 2016-06-02T06:35:30Z

what if i edit a document that is in an illegal place already, but is neither moved nor created? i think it would make sense to allow that - a user otherwise could have a hard time saving their changes and understanding what is happening. and the situation was illegal before the edit already...

dantleech · 2016-06-02T08:18:26Z

what if i edit a document that is in an illegal place already

Not sure .. if their tree is in an illegal state, then its broken, so I would expect an \Exception when editing an illegal child - otherwise nobody knows that it is broken until part of the system that relies on chldren being of a specific type recieves a class of the wrong type and throws an error at a critical moment.

But it does raise the issue of migration when restrictions are added to an existing model - but I would say that that excerise could be left to the user with the PhpcrMigrationsBundle.

dbu · 2016-06-02T08:22:46Z

what is the current code doing? i guess we definitely don't want to throw errors on loading. but on saving an edit i don't know which is better, error or not. i tend to say no error if its just property changes, but maybe its too difficult to separate from adding another child or a child to the illegal child or whatever, so an error sounds ok. we could do a command to validate the validity of the current tree, to help figure out if the migration worked as expected...

dantleech · 2016-06-02T08:28:01Z

we could do a command to validate the validity of the current tree, to
help figure out if the migration worked as expected...

yeah that crossed my mind too.. I would say that could be another PR.

dbu · 2016-06-02T08:50:41Z

we could do a command to validate the validity of the current tree, to
help figure out if the migration worked as expected...
yeah that crossed my mind too.. I would say that could be another PR.

yes, for sure. there are other things to check, like missing required
properties.

dantleech · 2016-06-03T13:13:53Z

@dbu have added the XML and YAML drivers and tests.

So it looks like this:

Annotation:

@Document(requiredClasses={"stdClass"})

XML:

<document ...>
    <required-class>stdClass</required-class>
    <!-- ... -->
</document>

YAML:

MyMapping:
    required_classes: [ "stdClass" ]

There remains the case where the user wants to explicitly declare that they want no children, which is not possible the XML or YAML drivers, we could add a "allow_children" boolean flag?

MyMapping:
    allow_children: false

dantleech · 2016-06-03T13:43:28Z

Made the benchmark, no noticable difference:

+------------------+--------------------------------------+------------------------+
| subject          | vcs_branch:children_restriction:mean | vcs_branch:master:mean |
+------------------+--------------------------------------+------------------------+
| benchPersistTwo  | 40.43ms                              | 39.67ms                |
| benchPersistMany | 65.65ms                              | 72.44ms                |
+------------------+--------------------------------------+------------------------+

vcs_branch: master
+------------------+-----+-------------+---------+---------+--------+
| subject          | its | mem         | mean    | mode    | rstdev |
+------------------+-----+-------------+---------+---------+--------+
| benchPersistTwo  | 10  | 11,248,728b | 39.67ms | 39.03ms | 11.71% |
| benchPersistMany | 10  | 11,403,322b | 72.44ms | 66.97ms | 15.04% |
+------------------+-----+-------------+---------+---------+--------+

vcs_branch: children_restriction
+------------------+-----+-------------+---------+---------+--------+
| subject          | its | mem         | mean    | mode    | rstdev |
+------------------+-----+-------------+---------+---------+--------+
| benchPersistTwo  | 10  | 11,264,174b | 40.43ms | 36.61ms | 17.80% |
| benchPersistMany | 10  | 11,419,214b | 65.65ms | 62.13ms | 9.41%  |
+------------------+-----+-------------+---------+---------+--------+

It uses a bit more memory however.

dbu · 2016-06-03T14:09:15Z

i don't like @Document(requiredClasses={...}). it feels like we would define the classes this document may have, which would make no sense but could be confusing. i think allowedChildClasses would be better.

in jackrabbit cnd the child types are defined on the child / children names. i think we have the same here. on an @Child/Children annotation, we don't need to repeat its about the children, but on the class level we should. requiredChildClasses feels like the children has to implement all of the classes, or we require children, but what we do is only allow children that are of one of the specified classes.

dbu · 2016-06-03T14:09:52Z

and thanks for the benchmark, looks like its not a noticable performance decrease. and its for write only, not for read so it should be pretty safe.

dbu · 2016-06-03T14:11:31Z

regarding forbidding all children: what if we allow to just have one entry with false and interpret that as not allowing children? in xml, it would be <allowed-child-class>false</...> which feels ok-ish. if you don't like it, a separate option would be ok. they would need to be sanity checked as the two things are mutually exclusive.

dantleech · 2016-06-03T14:13:02Z

and go with requiredClasses - i like to keep this as close to PHPCR as makes sense.

i don't like @document(requiredClasses={...})

:) but also fine to change it, its just for the sake of copying PHPCR.

dantleech · 2016-06-03T14:14:27Z

I think I prefer the explcit option, will update.

dantleech · 2016-06-16T07:56:54Z

@@ -1,9 +1,12 @@
 <phpunit colors="true" bootstrap="bootstrap.php" backupGlobals="false" cacheTokens="true">
    <php>
+        <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/>


May as well remove this

dbu · 2016-06-18T09:10:13Z

can you please rebase on master and then bump the branch-alias for master to 1.4? i think apart from that we are pretty much ready.

dantleech · 2016-06-18T09:17:14Z

Rebased and bumped the branch alias for this branch to 1.4-dev

wouterj · 2016-06-18T09:44:58Z

+        }
+
+        $childClasses = $this->getChildClasses();
+


if (0 === count($childClasses)) { ?

dantleech · 2016-06-20T07:30:49Z

@dbu good to merge?

dbu · 2016-06-20T07:31:46Z

+        $cm->setChildClasses(array('stdClass'));
+        $childCm = new ClassMetadata('stdClass');
+        $childCm->initializeReflection(new RuntimeReflectionService());
+        $result = $cm->assertValidChildClass($childCm);


i'd even remove the assignment here and just have $cm->assertValidChildClass($childCm);

dbu · 2016-06-20T07:32:33Z

i'd prefer to clean up that assertNull in the tests. apart from that its ready, yes. great job!

dantleech · 2016-06-20T11:15:59Z

ok, well if it could cause a compiler to crash then its not good :) removed the assertNull checks on the validation tests.

dbu · 2016-06-20T12:12:42Z

yay!

dantleech · 2016-06-20T12:18:09Z

\o/

On Mon, Jun 20, 2016 at 05:12:43AM -0700, David Buchmann wrote:

yay!

—
You are receiving this because you were mentioned.
Reply to this email directly, [1]view it on GitHub, or [2]mute the thread.

Reverse link: [3]unknown

References

Visible links

Support child restriction #706 (comment)

https://github.com/notifications/unsubscribe/AAgZcSlRi9klSdpQ-3NlTI8ul6LPb6_vks5qNoO7gaJpZM4Ipo4b

Support child restriction #706 (comment)

dantleech reviewed May 30, 2016
View reviewed changes

dantleech force-pushed the children_restriction branch from a2481f6 to 1e084bf Compare May 30, 2016 09:37

dantleech added the wip/poc label May 30, 2016

dbu reviewed Jun 2, 2016
View reviewed changes

dbu changed the title ~~Support child restriction~~ WIP: Support child restriction Jun 2, 2016

dantleech mentioned this pull request Jun 3, 2016

[WIP] Support for move and remove symfony-cmf/resource#23

Merged

dantleech force-pushed the children_restriction branch from 229c628 to ad0f215 Compare June 3, 2016 13:14

dantleech force-pushed the children_restriction branch from ad0f215 to 75e9f84 Compare June 5, 2016 08:54

dantleech force-pushed the children_restriction branch from c6f4c41 to a13b30b Compare June 16, 2016 07:35

dantleech reviewed Jun 16, 2016
View reviewed changes

dantleech force-pushed the children_restriction branch 2 times, most recently from 217299b to 3cde352 Compare June 16, 2016 07:58

dantleech force-pushed the children_restriction branch 2 times, most recently from 7390b49 to ad381e0 Compare June 18, 2016 09:16

wouterj reviewed Jun 18, 2016
View reviewed changes

dantleech force-pushed the children_restriction branch 2 times, most recently from dac5d49 to 9329d4b Compare June 18, 2016 15:04

dbu reviewed Jun 20, 2016
View reviewed changes

dbu changed the title ~~WIP: Support child restriction~~ Support child restriction Jun 20, 2016

Support child restriction and leaf status

aeaf6a8

dantleech force-pushed the children_restriction branch from 9329d4b to aeaf6a8 Compare June 20, 2016 11:15

dbu merged commit 23a0c6a into master Jun 20, 2016

dbu deleted the children_restriction branch June 20, 2016 12:12

lsmith77 removed the wip/poc label Jun 20, 2016

dbu mentioned this pull request Apr 3, 2018

document childclasses and isleaf mapping #781

Merged

Uh oh!

Conversation

dantleech commented May 30, 2016 • edited by dbu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

Alternatives

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu Jun 3, 2016 via email

Choose a reason for hiding this comment

Uh oh!

dantleech commented May 30, 2016

Uh oh!

dbu commented May 31, 2016

Uh oh!

dantleech commented May 31, 2016

Uh oh!

dbu commented Jun 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dantleech commented Jun 1, 2016

Uh oh!

dbu commented Jun 2, 2016 via email

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 Jun 2, 2016 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu Jun 3, 2016 via email

Choose a reason for hiding this comment

Uh oh!

dbu commented Jun 2, 2016

Uh oh!

dantleech commented Jun 2, 2016

Uh oh!

dbu commented Jun 2, 2016 via email

Uh oh!

dantleech commented Jun 2, 2016

Uh oh!

dbu commented Jun 2, 2016

Uh oh!

dantleech commented Jun 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dantleech commented Jun 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbu commented Jun 3, 2016

Uh oh!

dbu commented Jun 3, 2016

Uh oh!

dbu commented Jun 3, 2016

Uh oh!

dantleech commented Jun 3, 2016

Uh oh!

dantleech commented Jun 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jun 18, 2016

Uh oh!

dantleech commented Jun 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

dantleech commented May 30, 2016 •

edited by dbu

Loading

dbu commented Jun 1, 2016 •

edited

Loading

dantleech commented Jun 3, 2016 •

edited

Loading

dantleech commented Jun 3, 2016 •

edited

Loading