[WIP][Mapping] Ported some of the yaml driver to use Symfony config #479

Closed
wants to merge 4 commits into
from

Projects

None yet

5 participants

@kimhemsoe
Member

Looking for some input. How much validation and normailization should i push to the configuration ? Should i use default values so we can remove alot of lines from the driver ?

Is the way im allowing to extend the configuration good enough for Gedmo and others (untested)?

and.. did i catch all the settings ?

@doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2078

@stof stof and 1 other commented on an outdated diff Oct 14, 2012
lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
@@ -671,11 +667,35 @@ private function columnToArray($fieldName, $column)
return $mapping;
}
+ private function getConfigTree()
+ {
+ if ($this->configTree) {
+ return $this->configTree;
+ }
+
+ $configuration = new YamlConfiguration();
+ $builder = $configuration->getConfigTreeBuilder();
+
+ foreach ($this->configurationExtensions as $extension) {
+ $extension->addConfiguration($builder);
@stof
stof Oct 14, 2012 Doctrine member

This will not work for Gedmo as it would only allow to add new children at the top lovel, not to modify existing ones

@kimhemsoe
kimhemsoe Oct 14, 2012 Doctrine member

What do you suggest ?

@kimhemsoe
kimhemsoe Oct 15, 2012 Doctrine member

So, since it does not allow to navigate the tree, is the only other solution then to allow extra keys everywhere ?

Would be sad if that were the only solution.

@stof
Member
stof commented Oct 15, 2012

you would have to give access to each node of the tree where you want to allow extension points. It would not be easy

@kimhemsoe
Member

That could end up being messy.

what points would gedmo need ?

fields, root ?

@kimhemsoe
Member

And assoctions it seems.

so.. would something like addConfigration($builder, $area) work ?

@stof
Member
stof commented Oct 15, 2012

Different methods would be better IMO as the logic is likely to be different for each area

@kimhemsoe
Member

ok, will try to cook that up. Will create a abstract aswell with the interface, so users are less sentive when a new method are getting added.

@kimhemsoe
Member

Would something like this work ?


interface ExtensionInterface
{
    public function addConfiguration(NodeDefinition $builder);

    public function addFieldConfiguration(NodeDefinition $builder);

    public function addOneToOneConfiguration(NodeDefinition $builder);

    public function addManyToOneConfiguration(NodeDefinition $builder);

    public function addOneToManyConfiguration(NodeDefinition $builder);

    public function addManyToManyConfiguration(NodeDefinition $builder);
}
@beberlei
Member

How about tests? :-)

@henrikbjorn henrikbjorn commented on an outdated diff Oct 16, 2012
lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
@@ -46,6 +51,14 @@ public function __construct($locator, $fileExtension = self::DEFAULT_FILE_EXTENS
}
/**
+ * @param \Doctrine\ORM\Mapping\Driver\Configuration\ExtensionInterface $extension
+ */
+ public function addCnfigurationExtension(ExtensionInterface $extension)
@henrikbjorn
henrikbjorn Oct 16, 2012

Typo in method name.

@kimhemsoe kimhemsoe commented on an outdated diff Oct 16, 2012
...RM/Mapping/Driver/Configuration/YamlConfiguration.php
+ ->append($this->getToOneNode('oneToOne'))
+ ->append($this->getToOneNode('manyToOne'))
+ ->append($this->getManyToManyNode())
+ ->append($this->getOneToManyNode())
+ ->append($this->getNamedQueriesNode())
+ ->append($this->getLifecycleCallbacksNode())
+ ->append($this->getNamedNativeQueriesNode())
+ ->append($this->getConstraintsNode('uniqueConstraints'))
+ ->append($this->getConstraintsNode('indexes'))
+ ->append($this->getSqlResultSetMappingsNode())
+ ->end()
+ ->end();
+
+ foreach ($this->configurationExtensions as $extension) {
+ $extension->addConfiguration($prototype);
+ }
@kimhemsoe
kimhemsoe Oct 16, 2012 Doctrine member

Should we give access to the prototype like this or the chidren object ?

@kimhemsoe
Member

ping anyone todo a new review of this, rebased it from master, updated and added a simple test for the extension interface.

@kimhemsoe
Member

@beberlei ive updated to master and added very simple test. What is the chance of getting this in ?

@beberlei beberlei commented on an outdated diff Apr 22, 2013
...M/Mapping/Driver/Configuration/ExtensionInterface.php
@@ -0,0 +1,40 @@
+<?php
@beberlei
beberlei Apr 22, 2013 Doctrine member

I dont think we need the interface, when we have the abstract extension.

@beberlei beberlei commented on an outdated diff Apr 22, 2013
...RM/Mapping/Driver/Configuration/YamlConfiguration.php
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ORM\Mapping\Driver\Configuration;
+
+use Symfony\Component\Config\Definition\ConfigurationInterface;
+use Symfony\Component\Config\Definition\Builder\TreeBuilder;
+
+/**
+ * @author Kim Hemsø Rasmussen <kimhemsoe@gmail.com>
+ */
+class YamlConfiguration implements ConfigurationInterface
@beberlei
beberlei Apr 22, 2013 Doctrine member

Can you rename this to YamlMappingConfiguration?

@beberlei beberlei commented on the diff Apr 22, 2013
lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
@@ -699,11 +710,31 @@ private function columnToArray($fieldName, $column)
return $mapping;
}
+ private function getConfigTree()
+ {
+ if ($this->configTree) {
@beberlei
beberlei Apr 22, 2013 Doctrine member

Since Config is optional we cannot really check if persons used both Yaml and Config. Can you throw an exception if config is missing please?

@kimhemsoe
kimhemsoe Apr 22, 2013 Doctrine member

Not sure what you mean here. I check if i already have created the config tree and creating it if i havent.

@kimhemsoe
kimhemsoe Apr 23, 2013 Doctrine member

Should the SF Configuration be optional ?

@beberlei beberlei commented on an outdated diff Apr 22, 2013
lib/vendor/Symfony/Component/Config
@@ -0,0 +1 @@
+Subproject commit dbfadc6995574df68ad9b6c7e56a68b4bd9d0fa3
@beberlei
beberlei Apr 22, 2013 Doctrine member

We don't need submodules anymore. Can you remove it?

@kimhemsoe
Member

@beberlei If most details is sorted out here. i will try to implement this for gedmo. What are the chances of getting this in 2.4 ?

@beberlei
Member

@kimhemsoe 0, 2.4 is feature freeze already

@kimhemsoe
Member

ok, well.. that is what you get from postproning something for 6 months :P

I will look into howto implement this for gedmo for the next release. Is everything fine in is PR for now ?

@kimhemsoe
Member

@beberlei Should i rebase this, now that 2.4 is out ?

@kimhemsoe
Member

@beberlei ping

@kimhemsoe
Member

To many issues there will be anoying to solve.

@kimhemsoe kimhemsoe closed this Oct 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment