Skip to content

[WIP] add compiler pass for bundles to register mappings #177

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

Merged
merged 8 commits into from
May 4, 2013
Merged
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
146 changes: 146 additions & 0 deletions DependencyInjection/Compiler/DoctrineOrmMappingsPass.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
<?php

/*
* This file is part of the Doctrine Bundle
*
* (c) Fabien Potencier <[email protected]>
* (c) Doctrine Project, Benjamin Eberlei <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Doctrine\Bundle\DoctrineBundle\DependencyInjection\Compiler;

use Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterMappingsPass;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Reference;

/**
* Class for Symfony bundles to configure mappings for model classes not in the
* automapped folder.
*
* @author David Buchmann <[email protected]>
*/
class DoctrineOrmMappingsPass extends RegisterMappingsPass
{
/**
* You should not directly instantiate this class but use one of the
* factory methods.
*
* @param Definition|Reference $driver the driver to use
* @param array $namespaces list of namespaces this driver should handle.
* @param string[] $managerParameters ordered list of container parameters that may
* provide the name of the manager to register the mappings for. The first non-empty name
* is used, the others skipped.
* @param bool $enabledParameter if specified, the compiler pass only executes
* if this parameter exists in the service container.
*/
public function __construct($driver, $namespaces, array $managerParameters, $enabledParameter = false)
{
$managerParameters[] = 'doctrine.default_entity_manager';
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong to me. It should be enabled for the default manager if one of the other parameters has a null value, not in all cases

Copy link
Member Author

Choose a reason for hiding this comment

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

managerParameters is the list of container parameters to check. if i.e. the fos_user.doctrine_manager parameter has a null value, we need to fall back to this. at the point of creating the compiler pass we do not know yet if the parameter is going to be null or not. we only use this parameter if the other was null in https://github.com/symfony/symfony/pull/7755/files#L0R125

Copy link
Member

Choose a reason for hiding this comment

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

ah, it is registering it only on 1 manager, the first found. The phpdoc of the argument should be improved then

Copy link
Member Author

Choose a reason for hiding this comment

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

i improved the doc now

parent::__construct(
$driver,
$namespaces,
$managerParameters,
'doctrine.orm.%s_metadata_driver',
$enabledParameter
);

}

/**
* @param array $mappings Hashmap of directory path to namespace
* @param string[] $managerParameters List of parameters that could which object manager name
* your bundle uses. This compiler pass will automatically
* append the parameter name for the default entity manager
* to this list.
* @param string $enabledParameter Service container parameter that must be present to
* enable the mapping. Set to false to not do any check,
* optional.
*/
public static function createXmlMappingDriver(array $mappings, array $managerParameters = array(), $enabledParameter = false)
{
$arguments = array($mappings, '.orm.xml');
$locator = new Definition('Doctrine\Common\Persistence\Mapping\Driver\SymfonyFileLocator', $arguments);
$driver = new Definition('Doctrine\ORM\Mapping\Driver\XmlDriver', array($locator));
Copy link
Member

Choose a reason for hiding this comment

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

IMO, the definition should be marked as private so that the driver can get inlined when optimizing the container

Copy link
Member Author

Choose a reason for hiding this comment

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

i never register that as a service but inject it directly, instead of a reference. does that not make it private automatically? if not, i will change it of course.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, it does indeed


return new DoctrineOrmMappingsPass($driver, $mappings, $managerParameters, $enabledParameter);
Copy link
Member Author

Choose a reason for hiding this comment

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

@stof said: looks wrong to me. You are telling that $mappings is a hashmap of path to namespace and then passing it in a constructor argument documented as expecting a list of namespaces

i replied: what should i do instead? array_unique(array_value($mappings)) ? otherwise the value of both arrays are the same: namespaces

afaikt this is the only remaining open issue on this PR. anything else? any input on this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

@stof do we need to do something about the $mappings array here or is it fine?

}

/**
* @param array $mappings Hashmap of directory path to namespace
* @param string[] $managerParameters List of parameters that could which object manager name
* your bundle uses. This compiler pass will automatically
* append the parameter name for the default entity manager
* to this list.
* @param string $enabledParameter Service container parameter that must be present to
* enable the mapping. Set to false to not do any check,
* optional.
*/
public static function createYamlMappingDriver(array $mappings, array $managerParameters = array(), $enabledParameter = false)
{
$arguments = array($mappings, '.orm.yml');
$locator = new Definition('Doctrine\Common\Persistence\Mapping\Driver\SymfonyFileLocator', $arguments);
$driver = new Definition('Doctrine\ORM\Mapping\Driver\YamlDriver', array($locator));

return new DoctrineOrmMappingsPass($driver, $mappings, $managerParameters, $enabledParameter);
}

/**
* @param array $mappings Hashmap of directory path to namespace
* @param string[] $managerParameters List of parameters that could which object manager name
* your bundle uses. This compiler pass will automatically
* append the parameter name for the default entity manager
* to this list.
* @param string $enabledParameter Service container parameter that must be present to
* enable the mapping. Set to false to not do any check,
* optional.
*/
public static function createPhpMappingDriver(array $mappings, array $managerParameters = array(), $enabledParameter = false)
{
$arguments = array($mappings, '.php');
$locator = new Definition('Doctrine\Common\Persistence\Mapping\Driver\SymfonyFileLocator', $arguments);
$driver = new Definition('Doctrine\Common\Persistence\Mapping\Driver\PHPDriver', array($locator));

return new DoctrineOrmMappingsPass($driver, $mappings, $managerParameters, $enabledParameter);
}

/**
* @param array $namespaces List of namespaces that are handled with annotation mapping
* @param array $directories List of directories to look for annotated classes
* @param string[] $managerParameters List of parameters that could which object manager name
* your bundle uses. This compiler pass will automatically
* append the parameter name for the default entity manager
* to this list.
* @param string $enabledParameter Service container parameter that must be present to
* enable the mapping. Set to false to not do any check,
* optional.
*/
public static function createAnnotationMappingDriver(array $namespaces, array $directories, array $managerParameters = array(), $enabledParameter = false)
{
$reader = new Reference('doctrine.orm.metadata.annotation_reader');
$driver = new Definition('Doctrine\ORM\Mapping\Driver\AnnotationDriver', array($reader, $directories));

return new DoctrineOrmMappingsPass($driver, $namespaces, $managerParameters, $enabledParameter);
}

/**
* @param array $namespaces List of namespaces that are handled with static php mapping
* @param array $directories List of directories to look for static php mapping files
* @param string[] $managerParameters List of parameters that could which object manager name
* your bundle uses. This compiler pass will automatically
* append the parameter name for the default entity manager
* to this list.
* @param string $enabledParameter Service container parameter that must be present to
* enable the mapping. Set to false to not do any check,
* optional.
*/
public static function createStaticPhpMappingDriver(array $namespaces, array $directories, array $managerParameters = array(), $enabledParameter = false)
{
$driver = new Definition('Doctrine\Common\Persistence\Mapping\Driver\StaticPHPDriver', array($directories));
Copy link
Member Author

Choose a reason for hiding this comment

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

so this and the PHPDriver are actually in doctrine commons. i wonder if we could move the factory methods for those into the bridge? it will be copy-paste, except for the actual compiler pass class instantiated. hm. sounds like cludgy code - but if we keep it here, we will copy-paste to mongo/couch/phpcr

Copy link
Member

Choose a reason for hiding this comment

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

If the same code will end up existing in multiple bundles, that sounds like a prime candidate for the bridge.

Copy link
Member Author

Choose a reason for hiding this comment

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

problem is its not exactly the same, we need to know what to
instantiate. i could add something like
createStaticPhpMappingDriverDefinition to the bridge and keep the new
in the extending classes. or else pass the class as argument and do new $class if this is not considered bad style.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize how tiny these methods were. Were you considering moving all of these to the bridge, or just createStaticPhpMappingDriver()?

Copy link
Member Author

Choose a reason for hiding this comment

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

staticphpdriver and phpdriver are the same classes for all doctrines. the rest is specific anyways. but yeah i guess they are so small that its not worth it

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is worth moving it to the bridge for a single line

Copy link
Member Author

Choose a reason for hiding this comment

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

right, agree. i will copy it to the compiler passes of mongo/couch/phpcr then.


return new DoctrineOrmMappingsPass($driver, $namespaces, $managerParameters, $enabledParameter);
}
}