Made the Annotation builder automatically pass the EntityManager to Form Elements which need it. #180

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

tomphp commented Feb 19, 2013

Currently the only way I have found to get my

\DoctrineORMModule\Form\Element\EntitySelect

elements to populate is, after creating the form using the AnnotationBuilder, calling

$form->get('element_name')->setOptions(array('object_manager', $entityManager);

I find this pretty messy so this little mod does it automatically.

Hope you like it.

Owner

Ocramius commented Feb 19, 2013

@tomphp I do like the idea, but such stuff needs tests :\

Ping @bakura10

@Ocramius Ocramius commented on the diff Feb 19, 2013

...Module/Form/Annotation/ElementAnnotationsListener.php
+ }
+
+ if (!in_array(
+ $annotation->getType(),
+ array(
+ 'DoctrineORMModule\Form\Element\EntityMultiCheckbox',
+ 'DoctrineORMModule\Form\Element\EntityRadio',
+ 'DoctrineORMModule\Form\Element\EntitySelect',
+ )
+ )) {
+ return;
+ }
+
+ $elementSpec = $e->getParam('elementSpec');
+
+ $elementSpec['spec']['options']['object_manager'] = $this->em;
@Ocramius

Ocramius Feb 19, 2013

Owner

No side effects?

@tomphp

tomphp Feb 19, 2013

None that I'm aware of?

@Ocramius

Ocramius Feb 19, 2013

Owner

That's exactly what I mean: what does this line do? :)

@tomphp

tomphp Feb 19, 2013

This is the line which makes it all work. In order for EntityMultiCheckbox, EntityRadio & EntitySelect to work they required access to the EntityManager so they can fetch the options from the database, this is provided by an option called object_manager (see \DoctrineModule\Form\Element\Proxy). Here I am setting that option.

It saves having to call

$form->get('element_name')->setOptions(array('object_manager', $entityManager);

after the form has been created.

@Ocramius

Ocramius Feb 19, 2013

Owner

$elementSpec is not retrieved byref as far as I know

@tomphp

tomphp Feb 19, 2013

Presumably when not using Annotation you would do something like

$form->add(array(
    'name' => 'entity_name',
    'type' => 'DoctrineORMModule\Form\Element\EntitySelect',
    'options' => array(
            'object_manager' => $entityManager,
    )
));

However when using Annotations to build a form you cannot set object_manager to an instance in the annotation. So here I'm getting the AnnotationBuilder to do it for these types of Element.

@tomphp

tomphp Feb 19, 2013

@Ocramius I did wander about that but it seems to be working fine. Even though $elementSpec may not be retrieved by ref unless the value in object_manager is cloned it will still be a ref to the same object (since its an object and not a scalar) so it should be safe.

@Ocramius

Ocramius Feb 20, 2013

Owner

Ah, $elementSpec is an object?

@tomphp

tomphp Feb 20, 2013

I was talking about the EntityManager object however $elementSpec is indeed an instance of ArrayObject ;)

@tomphp

tomphp Feb 21, 2013

Oh by the way if you're worried about assigning to $elementSpec then take look at the other handler methods in this class you'll see that several of the assign to $elementSpec is the same way ;-)

tomphp commented Feb 19, 2013

as in you want me to write some unit tests or as in the idea/method needs testing?

Owner

Ocramius commented Feb 19, 2013

I stopped merging PRs without tests :P

tomphp commented Feb 19, 2013

No problem, I did look at the tests there and so much was uncovered so I wasn't sure if I should do it or not. Will get on to it now...back in 10mins or so ;)

Member

bakura10 commented Feb 19, 2013

Speaking about it, I suppose most of the AnnotationBuilder could be moved to DotrineModule.

tomphp commented Feb 20, 2013

Where are we at with this? Is it still the question of $elementSpec which is the sticking point or is there anything else I need to do?

tomphp commented Mar 1, 2013

Is there anything I need to do with this to get it accepted or is the concept still in question?

Owner

Ocramius commented Mar 28, 2013

@tomphp can you cross-check #193?

tomphp commented Mar 28, 2013

@Ocramius by the looks of it #193 does what this was supposed to do (and more) so this should probably be closed?

Owner

Ocramius commented Mar 29, 2013

@tomphp I can merge your test into #193, that one is OK IMO.

Consider that the feature (regardless if this one or the one in #193) lands in 0.8

tomphp commented Mar 29, 2013

@Ocramius are you talking about DoctrineORMModuleTest/Form/ElementAnnotationsListenerTest?

If so all I really added to that was a test for the extra method I created so I don't think it's relevant to #193

Unless I'm misunderstanding or missing something?

Owner

Ocramius commented Mar 29, 2013

@tomphp nvm, I saw what you mean. I think it's just going to be funny to handle the merge conflicts, but that's up to me :)

Owner

Ocramius commented Mar 29, 2013

Scheduled for 0.8

tomphp commented Mar 29, 2013

@Ocramius do you think this PR is needed? To me it looks like lines 88 and 112 of `src/DoctrineORMModule/Form/Annotation/ElementAnnotationsListener.php' in #193 handle the functionality that I was aiming to provide with this PR. I think this PR is probable redundant?

Unless I'm missing something I think this PR should be closed and just #193 merged?

Owner

Ocramius commented Mar 29, 2013

@tomphp I will keep the test

Owner

Ocramius commented Jun 11, 2013

Sorry about the awfully late response :( I'm going to close this since the entire annotation listener was re-written to use metadata in #233

Ocramius closed this Jun 11, 2013

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