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

Already on GitHub? Sign in to your account

Configurable array key #48

Merged
merged 3 commits into from Apr 6, 2013

Conversation

Projects
None yet
3 participants
Contributor

elHornair commented Apr 4, 2013

Let the user choose wether to use the uuid or the path of the node as an array key. This PR includes no BC break

@dbu dbu commented on an outdated diff Apr 4, 2013

...sformer/ReferenceManyCollectionToArrayTransformer.php
@@ -3,6 +3,7 @@
namespace Doctrine\Bundle\PHPCRBundle\Form\DataTransformer;
use Symfony\Component\Form\DataTransformerInterface;
+use Joiz\CmsBundle\Document\Show;
@dbu

dbu Apr 4, 2013

Member

this does not belong here

@dbu dbu commented on an outdated diff Apr 4, 2013

...sformer/ReferenceManyCollectionToArrayTransformer.php
@@ -38,7 +46,7 @@ public function transform($collection)
$arr = array();
foreach ($collection as $item) {
- $arr[] = $item->getPath();
+ $arr[] = $this->useUuidAsArrayKey ? $item->getNode()->getPropertyValue('jcr:uuid') : $item->getPath();
@dbu

dbu Apr 4, 2013

Member

you should use NodeInterface::getIdentifier instead of getPropertyValue(jcr:uuid)

@dbu dbu and 1 other commented on an outdated diff Apr 4, 2013

Form/Type/PHPCRODMReferenceCollectionType.php
@@ -47,6 +47,11 @@ public function setDefaultOptions(OptionsResolverInterface $resolver)
{
parent::setDefaultOptions($resolver);
$resolver->setRequired((array('referenced_class')));
+ $resolver->setOptional(array('use_uuid_as_array_key'));
+
+ $resolver->setDefaults(array(
+ 'use_uuid_as_array_key' => false,
@dbu

dbu Apr 4, 2013

Member

i am not too happy with the name of this option. it is quite long. but i can't think of a much better name. by_uuid would be short but maybe too short. have any ideas?

@dantleech

dantleech Apr 4, 2013

Contributor

Maye "key" or "index_by" as enum with values "path" or "uuid" with default. (sorry no special chars on phone)

Member

dbu commented Apr 4, 2013

sounds doable, i commented on some issues. i'd love to hear the exact use case why we need the option - but as we are talking about references, maybe the uuid would make more sense even as a default?

btw, are you sure that for the reverse transform we do not need to distinguish the cases?

Contributor

elHornair commented Apr 6, 2013

Thx for the feedback, did the corrections.

We need it because sometimes a CMF user wants to identify his content by path, sometimes by UUID.
Yes, I'm sure - ReferenceManyCollection can take both, UUIDs and paths, so it doesn't matter, what we pass it in the reverse transform.

Anything else that should be adapted before it can be merged?

@dantleech dantleech commented on an outdated diff Apr 6, 2013

...sformer/ReferenceManyCollectionToArrayTransformer.php
*/
- function __construct(DocumentManager $dm, $referencedClass)
+ function __construct(DocumentManager $dm, $referencedClass, $key)
@dantleech

dantleech Apr 6, 2013

Contributor

I'm not sure if we should have a default value, i.e. KEY_UUID or KEY_PATH, @dbu ?

@dantleech dantleech commented on an outdated diff Apr 6, 2013

...sformer/ReferenceManyCollectionToArrayTransformer.php
@@ -9,6 +9,9 @@
class ReferenceManyCollectionToArrayTransformer implements DataTransformerInterface
{
@dantleech

dantleech Apr 6, 2013

Contributor

I don't think we need this blank line

dbu added a commit that referenced this pull request Apr 6, 2013

@dbu dbu merged commit cf41680 into doctrine:master Apr 6, 2013

Member

dbu commented Apr 6, 2013

thanks alain! and thanks dan for the feedbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment