[Form] Refactored EntityChoiceList and ModelChoiceList #6627

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

alvarezmario commented Jan 8, 2013

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Fixes the following tickets: #6618

This commit adds a new abstract base class for both the EntityChoiceList and ModelChoiceList classes.

alvarezmario added some commits Jan 8, 2013

@alvarezmario alvarezmario [Form] Refactored EntityChoiceList and ModelChoiceList
This commit adds a new abstract base class for both the
EntityChoiceList and ModelChoiceList classes.
684b621
@alvarezmario alvarezmario Fixed typo 363b376
@alvarezmario alvarezmario Fixed docblocks df7cc1d
Contributor

mvrhov commented Jan 9, 2013

I would name it ModelChoiceList insetead of ORMChoiceList.
For Propel ModelChoiceList I would then alias the base class..

@stof stof and 1 other commented on an outdated diff Jan 9, 2013

...nent/Form/Extension/Core/ChoiceList/ORMChoiceList.php
+ {
+ if ($this->optimizedIdentifierCheck()) {
+ return (string) current($this->getIdentifierValues($object));
+ }
+
+ return parent::createValue($object);
+ }
+
+ /**
+ * This optimization does not check choices for existence
+ *
+ * Should be implemented by child classes.
+ *
+ * @return Boolean
+ */
+ abstract protected function optimizedIdentifierCheck();
@stof

stof Jan 9, 2013

Member

I would rename it to something like canOptimizeIdentifier()

@alvarezmario

alvarezmario Jan 9, 2013

Contributor

Indeed the name was not the best. :)

@stof stof commented on the diff Jan 9, 2013

...nent/Form/Extension/Core/ChoiceList/ORMChoiceList.php
+ * Loads the list with ORM objects.
+ */
+ abstract protected function load();
+
+ /**
+ * Returns the values of the identifier fields of an object.
+ *
+ * The ORM must know about this object, that is, the object must already
+ * be persisted or added to the identity map before. Otherwise an
+ * exception is thrown.
+ *
+ * @param object $object The object for which to get the identifier
+ *
+ * @return array The identifier values
+ *
+ * @throws Exception If the object does not exist in ORM's identity map
@stof

stof Jan 9, 2013

Member

The comments talking about the "identity map" should be reworded. Some ORMs may not have an identity map (or have a different name for it)

@alvarezmario

alvarezmario Jan 9, 2013

Contributor

@stof what name could be more generic?

Contributor

alvarezmario commented Jan 9, 2013

@stof @bschussek What do you think about the class name? It's fine like this or @mvrhov suggestion is better?

Member

stof commented Jan 9, 2013

I don't like the ORMChoiceList name. This base class is not limited to ORMs. For instance, the EntityChoiceList class is used for any project based on the Doctrine Common interfaces (so the ORM but also the MongoDB ODM, the PHPCR ODM and the CouchDB ODM)

Contributor

alvarezmario commented Jan 9, 2013

'ModelChoiceList' is a better option them?

2013/1/9 Christophe Coevoet notifications@github.com

I don't like the ORMChoiceList name. This base class is not limited to
ORMs. For instance, the EntityChoiceList class is used for any project
based on the Doctrine Common interfaces (so the ORM but also the MongoDB
ODM, the PHPCR ODM and the CouchDB ODM)


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/6627#issuecomment-12063991.

Member

webmozart commented Feb 21, 2013

See also #3240.

I think the first step of this refactoring should add lazy loading to ChoiceList. Then we can deprecate LazyChoiceList and remove the loading functionality from EntityChoiceList and ModelChoiceList. Once we have done that, we can see what more duplicated functionality remains and whether it warrants to introduce a new intermediate class (here called ORMChoiceList).

Member

webmozart commented Apr 19, 2013

Since there is no more activity on this PR, I will close it. I would be very happy if you found the time to move the lazy loading to ChoiceList as described above. In this case, please open a new PR. :)

webmozart closed this Apr 19, 2013

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