Fix root option on TaxonChoiceType #7254

Pitoune · 2017-01-13T11:28:43Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	yes
Related tickets
License	MIT

lchrusciel · 2017-01-13T15:22:20Z

src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonChoiceType.php

@@ -81,7 +80,14 @@ public function configureOptions(OptionsResolver $resolver)
        $resolver
            ->setDefaults([
                'choices' => function (Options $options) {
-                    $taxons = $this->taxonRepository->findNodesTreeSorted();
+                    if (null !== $options['root']) {
+                        if ($options['root'] instanceof TaxonInterface) {


Isn't it too much magic? Why don't we stay with only taxon code in root?

lchrusciel · 2017-01-13T15:22:47Z

src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonChoiceType.php

@@ -81,7 +80,14 @@ public function configureOptions(OptionsResolver $resolver)
        $resolver
            ->setDefaults([
                'choices' => function (Options $options) {
-                    $taxons = $this->taxonRepository->findNodesTreeSorted();
+                    if (null !== $options['root']) {


Could be extracted to private method

lchrusciel · 2017-01-13T15:23:03Z

src/Sylius/Bundle/TaxonomyBundle/Doctrine/ORM/TaxonRepository.php

@@ -43,6 +42,24 @@ public function findChildren($parentCode)
    /**
     * {@inheritdoc}
     */
+    public function findChildrenByRootCode($rootCode)
+    {
+        $queryBuilder = $this->createQueryBuilder('o');


Zales0123 · 2017-01-17T06:20:26Z

src/Sylius/Bundle/TaxonomyBundle/Doctrine/ORM/TaxonRepository.php

+                ->leftJoin('o.root', 'root')
+                ->andWhere('root.code = :code')
+            ;
+        } else {


We should avoid this nasty else statement. Would be a good to extract this to private method. But also there is a lot of logic in this if... So maybe two different methods, findChildren and findChildrenForRootParent?

That's what I did in the first place but @lchrusciel said the method findChildrenForRootParent was redundant with the findChildren one.

Sorry, I wasn't clear enough... I meant that the variable assignment is redundant... Sorry about that :)

return $this->createQueryBuilder('o') ->addSelect('child') ->leftJoin('o.children', 'child') ->leftJoin('o.parent', 'parent') ->andWhere('parent.code = :code') ;

Zales0123 · 2017-01-17T06:22:07Z

src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonChoiceType.php

+
+    /**
+     * @param null|string   $root
+     * @param null|callable $filter


string|null and callable|null. Also we should not align parameters vertically.

Zales0123 · 2017-01-17T06:24:16Z

src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonChoiceType.php

+     * @param null|string   $root
+     * @param null|callable $filter
+     *
+     * @return array|TaxonInterface[]


TaxonInterface[] would be sufficient, array| indicates as there could be something else ;)

Zales0123 · 2017-01-17T06:25:31Z

src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonChoiceType.php

+    {
+        if (null !== $root) {
+            $taxons = $this->taxonRepository->findChildren($root, true);
+        } else {


I know that another extraction would not be a good option (two methods for a simple case), but else does not look good too :/ How could we make it shiny and perfect?

What about adding a nullable argument $rootCode to method findNodesTreeSorted ? It means putting some logic in the repository but we only need an if without else.

Zales0123 · 2017-01-17T06:25:52Z

src/Sylius/Component/Taxonomy/Repository/TaxonRepositoryInterface.php

@@ -25,10 +25,11 @@
 {
    /**
     * @param string $parentCode
+     * @param bool   $isParentRoot


Again, do not indent parameters vertically.

pamil · 2017-01-17T08:30:06Z

src/Sylius/Bundle/TaxonomyBundle/Doctrine/ORM/TaxonRepository.php

@@ -23,20 +22,30 @@ class TaxonRepository extends EntityRepository implements TaxonRepositoryInterfa
    /**
     * {@inheritdoc}
     */
-    public function findChildren($parentCode)
+    public function findChildren($parentCode, $isParentRoot = false)


Why do we need this parameter?

findChildren('CODE', true) shouldn't return grandchildren etc.
It should be rather findChildren($parentCode) and findDescendants($rootCode).

michalmarcinkowski · 2017-01-18T11:51:15Z

src/Sylius/Bundle/TaxonomyBundle/Form/Type/TaxonChoiceType.php

-                    }
-
-                    return $taxons;
+                    return $this->getTaxons($options['root'], $options['filter']);


I would rename this option to root_code to avoid confusion.

Pitoune · 2017-01-30T10:14:28Z

@michalmarcinkowski did you have time to have a look on my last commit?

michalmarcinkowski · 2017-01-30T19:47:12Z

I'm not a fan of optional arguments, but creating a separate method for that doesn't seem a better choice. Thanks Pierre! 👍

pjedrzejewski added the Bug Fix label Jan 13, 2017

lchrusciel requested changes Jan 13, 2017

View reviewed changes

Pitoune force-pushed the feat/taxon-choice-type-root-option branch from 783f707 to 910bb12 Compare January 16, 2017 09:59

Zales0123 requested changes Jan 17, 2017

View reviewed changes

pamil suggested changes Jan 17, 2017

View reviewed changes

Pitoune force-pushed the feat/taxon-choice-type-root-option branch 4 times, most recently from 5308eda to 5f907b2 Compare January 17, 2017 11:21

michalmarcinkowski reviewed Jan 18, 2017

View reviewed changes

Pitoune force-pushed the feat/taxon-choice-type-root-option branch from 5f907b2 to d17ad26 Compare January 18, 2017 12:09

Fix root option on TaxonChoiceType

8e9a2bb

Pitoune force-pushed the feat/taxon-choice-type-root-option branch from d17ad26 to 8e9a2bb Compare January 30, 2017 08:50

lchrusciel approved these changes Jan 30, 2017

View reviewed changes

Zales0123 approved these changes Jan 30, 2017

View reviewed changes

pjedrzejewski added the BC Break PRs introducing BC breaks (do not even try to merge). label Jan 30, 2017

pamil approved these changes Jan 30, 2017

View reviewed changes

michalmarcinkowski merged commit 367bad5 into Sylius:master Jan 30, 2017

Pitoune deleted the feat/taxon-choice-type-root-option branch January 31, 2017 08:19

pamil mentioned this pull request Feb 1, 2017

Revert "Fix root option on TaxonChoiceType" #7392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix root option on TaxonChoiceType #7254

Fix root option on TaxonChoiceType #7254

Pitoune commented Jan 13, 2017 •

edited

Loading

lchrusciel Jan 13, 2017

lchrusciel Jan 13, 2017

lchrusciel Jan 13, 2017

Zales0123 Jan 17, 2017

Pitoune Jan 17, 2017

lchrusciel Jan 17, 2017 •

edited

Loading

Zales0123 Jan 17, 2017

Zales0123 Jan 17, 2017

Zales0123 Jan 17, 2017

Pitoune Jan 17, 2017

Zales0123 Jan 17, 2017

pamil Jan 17, 2017

pamil Jan 17, 2017

michalmarcinkowski Jan 18, 2017

Pitoune commented Jan 30, 2017

michalmarcinkowski commented Jan 30, 2017

Fix root option on TaxonChoiceType #7254

Fix root option on TaxonChoiceType #7254

Conversation

Pitoune commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchrusciel Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pitoune commented Jan 30, 2017

michalmarcinkowski commented Jan 30, 2017

Pitoune commented Jan 13, 2017 •

edited

Loading

lchrusciel Jan 17, 2017 •

edited

Loading