Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support child restriction #706

Merged
merged 1 commit into from
Jun 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
Changelog
=========

dev-master
----------

* **2016-06-08** [Feature] Allow children of a document to be restricted to a
certain class or forbidden.

1.3.1
-----

Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"bin": ["bin/phpcrodm", "bin/phpcrodm.php"],
"extra": {
"branch-alias": {
"dev-master": "1.3-dev"
"dev-master": "1.4-dev"
}
}
}
6 changes: 6 additions & 0 deletions doctrine-phpcr-odm-mapping.xsd
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@
<xs:anyAttribute namespace="##other"/>
</xs:complexType>

<xs:complexType name="child-class">
<xs:attribute name="name" type="xs:string"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbu changed to an attribute as there are no examples of node-value elements in Doctrine. /cc @wouterj

</xs:complexType>

<xs:complexType name="mixin">
<xs:attribute name="type" type="xs:NMTOKEN"/>
</xs:complexType>
Expand Down Expand Up @@ -138,6 +142,7 @@
<xs:element name="document-listeners" type="phpcr:document-listeners" minOccurs="0" maxOccurs="1" />
-->
<xs:element name="id" type="phpcr:id" minOccurs="0" maxOccurs="1" />
<xs:element name="child-class" type="phpcr:child-class" minOccurs="0" maxOccurs="unbounded" />
<xs:element name="uuid" type="phpcr:uuid" minOccurs="0" maxOccurs="1" />
<xs:element name="locale" type="phpcr:locale" minOccurs="0" maxOccurs="1" />
<xs:element name="field" type="phpcr:field" minOccurs="0" maxOccurs="unbounded"/>
Expand All @@ -159,6 +164,7 @@
<xs:attribute name="translator" type="xs:NMTOKEN"/>
<xs:attribute name="versionable" type="phpcr:versionable-type" />
<xs:attribute name="referenceable" type="xs:boolean" default="false" />
<xs:attribute name="is-leaf" type="xs:boolean" default="false" />
<xs:attribute name="node-type" type="xs:NMTOKEN" />
<xs:attribute name="repository-class" type="xs:string"/>
<xs:anyAttribute namespace="##other"/>
Expand Down
10 changes: 10 additions & 0 deletions lib/Doctrine/ODM/PHPCR/Mapping/Annotations/Document.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,14 @@ class Document
* @var boolean
*/
public $uniqueNodeType;

/**
* @var array
*/
public $childClasses = array();

/**
* @var boolean
*/
public $isLeaf;
}
112 changes: 112 additions & 0 deletions lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use Doctrine\Common\ClassLoader;
use Doctrine\Instantiator\Instantiator;
use Doctrine\Instantiator\InstantiatorInterface;
use Doctrine\ODM\PHPCR\Exception\OutOfBoundsException;

/**
* Metadata class
Expand Down Expand Up @@ -328,6 +329,23 @@ class ClassMetadata implements ClassMetadataInterface
*/
public $parentClasses = array();

/**
* READ-ONLY: Child class restrictions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the phpdoc should explain that if this list is empty, it means we don't check the classes of the children. to make a leaf node, use the explicit isLeaf field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

*
* If empty then any classes are permitted.
*
* @var array
*/
public $childClasses = array();

/**
* READ-ONLY: If the document should be act as a leaf-node and therefore
* not be allowed children.
*
* @var boolean
*/
public $isLeaf = false;

/**
* The inherited fields of this class
*
Expand Down Expand Up @@ -453,6 +471,57 @@ public function validateIdentifier()
}
}

/**
* Validate that childClasses is empty if isLeaf is true.
*
* @throws MappingException if there is a conflict between isLeaf and childClasses.
*/
public function validateChildClasses()
{
if (count($this->childClasses) > 0 && $this->isLeaf) {
throw new MappingException(sprintf(
'Cannot map a document as a leaf and define child classes for "%s"',
$this->name
));
}
}

/**
* Assert that the given class FQN can be a child of the document this
* metadata represents.
*
* @param string $classFqn
* @throws OutOfBoundsException
*/
public function assertValidChildClass(ClassMetadata $class)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbu moved the validation logic to the ClassMetadata.

{
if ($this->isLeaf()) {
throw new OutOfBoundsException(sprintf(
'Document "%s" has been mapped as a leaf. It cannot have children',
$this->name
));
}

$childClasses = $this->getChildClasses();

if (0 === count($childClasses)) {
return;
}

foreach ($childClasses as $childClass) {
if ($class->name === $childClass || $class->reflClass->isSubclassOf($childClass)) {
return;
}
}

throw new OutOfBoundsException(sprintf(
'Document "%s" does not allow children of type "%s". Allowed child classes "%s"',
$this->name,
$class->name,
implode('", "', $childClasses)
));
}

/**
* Validate whether this class needs to be referenceable.
*
Expand Down Expand Up @@ -1123,6 +1192,49 @@ public function getParentClasses()
return $this->parentClasses;
}

/**
* Return the class names or interfaces that children of this document must
* be an instance of.
*
* @return string[]
*/
public function getChildClasses()
{
return $this->childClasses;
}

/**
* Set the class names or interfaces that children of this document must be
* instance of.
*
* @param string[] $childClasses
*/
public function setChildClasses(array $childClasses)
{
$this->childClasses = $childClasses;
}

/**
* Return true if this is designated as a leaf node.
*
* @return bool
*/
public function isLeaf()
{
return $this->isLeaf;
}

/**
* Set if this document should act as a leaf node.
*
* @param bool $isLeaf
*/
public function setIsLeaf($isLeaf)
{
$this->isLeaf = $isLeaf;
}


/**
* Checks whether the class will generate an id via the repository.
*
Expand Down
1 change: 1 addition & 0 deletions lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ protected function validateRuntimeMetadata($class, $parent)
$class->validateIdentifier();
$class->validateReferenceable();
$class->validateReferences();
$class->validateChildClasses();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of strange that we don't have just one validate method on ClassMetadata and call that here, and keep the details of what needs to be validated local to ClassMetadata...

but not subject of this PR, so lets keep it like this here.

$class->validateLifecycleCallbacks($this->getReflectionService());
$class->validateTranslatables();

Expand Down
6 changes: 6 additions & 0 deletions lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$metadata->setTranslator($documentAnnot->translator);
}

if (array() !== $documentAnnot->childClasses) {
$metadata->setChildClasses($documentAnnot->childClasses);
}

$metadata->setIsLeaf($documentAnnot->isLeaf);

foreach ($reflClass->getProperties() as $property) {
if ($metadata->isInheritedField($property->name)
&& $metadata->name !== $property->getDeclaringClass()->getName()
Expand Down
20 changes: 20 additions & 0 deletions lib/Doctrine/ODM/PHPCR/Mapping/Driver/XmlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,18 @@ public function loadMetadataForClass($className, ClassMetadata $class)
$class->setUniqueNodeType((bool) $xmlRoot['uniqueNodeType']);
}

if (isset($xmlRoot['is-leaf'])) {
if (!in_array($value = $xmlRoot['is-leaf'], array('true', 'false'))) {
throw new MappingException(sprintf(
'Value of is-leaf must be "true" or "false", got "%s" for class "%s"',
$value, $className
));
}

$class->setIsLeaf($value == 'true' ? true : false);
}


if (isset($xmlRoot->mixins)) {
$mixins = array();
foreach ($xmlRoot->mixins->mixin as $mixin) {
Expand Down Expand Up @@ -259,6 +271,14 @@ public function loadMetadataForClass($className, ClassMetadata $class)
$class->mapField($mapping);
}

if (isset($xmlRoot->{'child-class'})) {
$childClasses = array();
foreach ($xmlRoot->{'child-class'} as $requiredClass) {
$childClasses[] = $requiredClass['name'];
}
$class->setChildClasses($childClasses);
}

$class->validateClassMapping();
}

Expand Down
8 changes: 8 additions & 0 deletions lib/Doctrine/ODM/PHPCR/Mapping/Driver/YamlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,14 @@ public function loadMetadataForClass($className, ClassMetadata $class)
}
}

if (isset($element['child_classes'])) {
$class->setChildClasses($element['child_classes']);
}

if (isset($element['is_leaf'])) {
$class->setIsLeaf($element['is_leaf']);
}

$class->validateClassMapping();
}

Expand Down
25 changes: 25 additions & 0 deletions lib/Doctrine/ODM/PHPCR/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
use PHPCR\Util\PathHelper;
use PHPCR\Util\NodeHelper;
use Jackalope\Session as JackalopeSession;
use Doctrine\ODM\PHPCR\Exception\OutOfBoundsException;

/**
* Unit of work class
Expand Down Expand Up @@ -2332,6 +2333,8 @@ private function executeInserts($documents)
}

$parentNode = $this->session->getNode(PathHelper::getParentPath($id));
$this->validateChildClass($parentNode, $class);

$nodename = PathHelper::getNodeName($id);
$node = $parentNode->addNode($nodename, $class->nodeType);
if ($class->node) {
Expand Down Expand Up @@ -2731,6 +2734,9 @@ private function executeMoves($documents)
);
}

$parentNode = $this->session->getNode(PathHelper::getParentPath($targetPath));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comes at a cost of loading another document

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. or rather a node. in some cases that node will already have been loaded (f.e. parent mappings always load the parent node), I will benchmark to check the performance cost.

$this->validateChildClass($parentNode, $class);

$this->session->move($sourcePath, $targetPath);

// update fields nodename, parentMapping and depth if they exist in this type
Expand Down Expand Up @@ -3968,4 +3974,23 @@ public function invokeGlobalEvent($eventName, EventArgs $event)
$this->eventManager->dispatchEvent($eventName, $event);
}
}

/**
* If the parent node has child restrictions, ensure that the given
* class name is within them.
*
* @param NodeInterface $parentNode
* @param string $classFqn
*/
private function validateChildClass(NodeInterface $parentNode, ClassMetadata $class)
{
$parentClass = $this->documentClassMapper->getClassName($this->dm, $parentNode);

if (null === $parentClass) {
return;
}

$metadata = $this->dm->getClassMetadata($parentClass);
$metadata->assertValidChildClass($class);
}
}
22 changes: 22 additions & 0 deletions tests/Doctrine/Tests/Models/CMS/CmsArticleFolder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

namespace Doctrine\Tests\Models\CMS;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;

/**
* @PHPCRODM\Document(childClasses={"Doctrine\Tests\Models\CMS\CmsArticle"})
*/
class CmsArticleFolder
{
/**
* @PHPCRODM\Id
*/
public $id;

/**
* @PHPCRODM\Children()
*/
public $articles;
}
26 changes: 26 additions & 0 deletions tests/Doctrine/Tests/Models/CMS/CmsBlogFolder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

namespace Doctrine\Tests\Models\CMS;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;

/**
* @PHPCRODM\Document(
* childClasses={
* "Doctrine\Tests\Models\CMS\CmsBlogPost"
* }
* )
*/
class CmsBlogFolder
{
/**
* @PHPCRODM\Id()
*/
public $id;

/**
* @PHPCRODM\Children()
*/
public $posts;
}
24 changes: 24 additions & 0 deletions tests/Doctrine/Tests/Models/CMS/CmsBlogInvalidChild.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add the header to styleci.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think styleci ignores the test folder.

namespace Documents;

namespace Doctrine\Tests\Models\CMS;

use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;
use Doctrine\ODM\PHPCR\DocumentRepository;
use Doctrine\ODM\PHPCR\Id\RepositoryIdInterface;

/**
* @PHPCRODM\Document()
*/
class CmsBlogInvalidChild
{
/** @PHPCRODM\Id(strategy="parent") */
public $id;

/** @PHPCRODM\NodeName() */
public $name;

/** @PHPCRODM\ParentDocument() */
public $parent;
}
Loading