Enable images for Taxonomy/Taxon #92

piotrantosik · 2013-05-20T22:19:20Z

See #83
Any comments welcome :)

pjedrzejewski · 2013-05-21T06:35:05Z

app/config/config.yml

@@ -78,17 +78,20 @@ liip_doctrine_cache:

 knp_gaufrette:
    adapters:
-        variant_image:
+        sylius_image:


pjedrzejewski · 2013-05-21T06:42:45Z

Nice work Piotr! I'll review this soon - the build is failing, on every page so perhaps there is some configuration bug.
I have few remark regarding that, I'll take a deeper look but I already see that - Every taxonomy always has it's root Taxon, so it would be enough to have image on Taxon entity, for Taxonomy (root) it would available through that taxon. Also the listener code looks a bit complex, if there are no specs for it, then it's my fault. :)

Thanks!

stloyd · 2013-05-21T06:47:22Z

src/Sylius/Bundle/CoreBundle/EventListener/ImageUploadListener.php

+            }
+        }
+        else {
+            if (null === $subject->getId()) {


This if can be merged with elseif below, and probably as replacement for else line above.

piotrantosik · 2013-05-22T08:19:20Z

@pjedrzejewski so remove image for Taxonomy - we must use image from root Taxon,you agree?

stloyd · 2013-05-22T08:22:18Z

src/Sylius/Bundle/CoreBundle/EventListener/ImageUploadListener.php

+            foreach ($variant->getImages() as $image) {
+                if (null === $image->getId()) {
+                    $this->uploader->upload($image);
+                }
            }
        }


} elseif (null === $subject->getId() || (($subject instanceof TaxonInterface || $subject instanceof TaxonomyInterface) && $subject->hasFile())) { $this->uploader->upload($subject); }

Actually, I think we could simply have 2 separate methods on this listener. One for products, one for taxons, this will simplify method complexity by removing all this conditions. uploadProductImage & uploadTaxonImage. This would involve simple change in listeners config. @Antek88 @stloyd

@pjedrzejewski TBH. I didn't check code in such details =) Just looked at how it looks etc. =)

@pjedrzejewski maybe this is a good time to create SyliusFileUploader? Universal tool that handle file uploading with doctrine entities?

pjedrzejewski · 2013-05-22T08:29:52Z

@Antek88 Yes, exactly! For taxonomy we will simply use $taxonomy->getRoot() to get the root taxon and then the image. But I think we can also introduce proxy methods like $taxonomy->getImagePath().

And btw. the getFile and getPath methods alone can be confusing on Taxon, could we rename them to getImagePath() and getImageFile(), then it's clear, what do you think?

pjedrzejewski · 2013-05-22T08:30:54Z

To upload image for root taxon on Taxonomy form, you can simply change the property_path option of the file field. So it will save the image to root taxon, not the taxonomy which will no longer have the image / path properties.

piotrantosik · 2013-05-22T21:13:35Z

@pjedrzejewski @stloyd ping

stloyd · 2013-05-22T21:19:46Z

src/Sylius/Bundle/CoreBundle/EventListener/ImageUploadListener.php

+            throw new \InvalidArgumentException('TaxonomyInterface expected.');
+        }
+
+        if ($subject->getRoot()->hasImageFile()) {


What if root is empty? (not sure it's possible =)) Or what if file is already uploaded in root?

root is always created:

$this->setRoot(new Taxon());

if file already exist uploader firstly remove file and upload new:

if (null !== $image->getImagePath()) { $this->remove($image->getImagePath()); }

pjedrzejewski · 2013-05-23T06:56:18Z

Only problems I see is the removing typehint (maybe there is a reason but I'm pretty sure we should do it, maybe change the interface) and that during rebase, you reverted some other PR's.

stloyd · 2013-05-23T08:16:26Z

src/Sylius/Bundle/WebBundle/Menu/FrontendMenuBuilder.php

@@ -161,6 +161,9 @@ public function createTaxonomiesMenu(Request $request)

        foreach ($taxonomies as $taxonomy) {
            $child = $menu->addChild($taxonomy->getName(), $childOptions);
+            if ($taxonomy->getRoot()->getPath()) {


IMO you should add hasPath() as you already have hasFile() method, or at least use variable here to avoid twice get* call (same below).

…image" This reverts commit 284b0d0a2a448b11ca9b9fef1a6d5d9638ca4320, reversing changes made to 553eea88539ffd170228e12793e60beabf3a95f3.

piotrantosik · 2013-05-31T19:22:19Z

@pjedrzejewski ping, tests fixed 😄

pjedrzejewski · 2013-05-31T19:42:01Z

src/Sylius/Bundle/CoreBundle/Resources/config/doctrine/Taxonomy.orm.xml

+        </many-to-one>
+    </entity>
+
+</doctrine-mapping>


Do we need to re-define this if we add image only to taxon?

We need this because in DefaultTaxonomy (Taxonomies bundle) class we have:

$this->setRoot(new DefaultTaxon());

and after remove Taxonomy mapping from core bundle i have error when creating new taxonomy:

[Doctrine\ORM\ORMException] Found entity of type Sylius\Bundle\TaxonomiesBundle\Entity\DefaultTaxon on association Sylius\Bundle\TaxonomiesBundle\Entity\DefaultTaxonom y#root, but expecting Sylius\Bundle\CoreBundle\Entity\Taxon

Enable images for Taxonomy/Taxon

pjedrzejewski · 2013-05-31T21:27:21Z

Woohoo, thank you Piotr! Great work! 👍

[CJMAX-82] Added more fields to Customer

pjedrzejewski reviewed May 21, 2013
View reviewed changes

app/config/config.yml

@@ -78,17 +78,20 @@ liip_doctrine_cache:

knp_gaufrette:

adapters:

variant_image:

sylius_image:

Copy link

Member

pjedrzejewski May 21, 2013

Choose a reason for hiding this comment

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

👍

stloyd reviewed May 21, 2013
View reviewed changes

stloyd reviewed May 22, 2013
View reviewed changes

stloyd reviewed May 23, 2013
View reviewed changes

piotrantosik added 3 commits May 31, 2013 19:48

Enable images for Taxonomy/Taxon

9df22bc

add spec to ImageUploadListener

3330458

images for taxon/taxonomy v2

aab7027

piotrantosik added 10 commits May 31, 2013 19:51

use one imageinterface and one uploader

e0b78ee

revert typehint

0f47466

Revert "Merge branch 'master' of github.com:Sylius/Sylius into taxon_…

634ecbf

…image" This reverts commit 284b0d0a2a448b11ca9b9fef1a6d5d9638ca4320, reversing changes made to 553eea88539ffd170228e12793e60beabf3a95f3.

revert bad rebase

9c10927

fix css style

8ba0c0d

fix image uploader

4a2e872

fix tests

f4bc5f3

ass hasPath to taxon & image

f11b5dc

css class

1360214

small fix

0fd9ea9

pjedrzejewski reviewed May 31, 2013
View reviewed changes

update ImageUploaderInterface

f27c0c6

pjedrzejewski pushed a commit that referenced this pull request May 31, 2013

Merge pull request #92 from Antek88/taxon_image

cbefbd4

Enable images for Taxonomy/Taxon

pjedrzejewski merged commit cbefbd4 into Sylius:master May 31, 2013

pjedrzejewski mentioned this pull request Jun 16, 2013

Suggestion: Enable images for Taxonomies or Categories. #83

Closed

okwinza mentioned this pull request May 24, 2016

[WIP] Taxon images refactoring #5102

Closed

8 tasks

CoderMaggie pushed a commit to CoderMaggie/Sylius that referenced this pull request Jun 1, 2016

Merge pull request Sylius#92 from Lakion/upgrade-customer

5fcb470

[CJMAX-82] Added more fields to Customer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable images for Taxonomy/Taxon #92

Enable images for Taxonomy/Taxon #92

piotrantosik commented May 20, 2013

pjedrzejewski May 21, 2013

pjedrzejewski commented May 21, 2013

stloyd May 21, 2013

piotrantosik commented May 22, 2013

stloyd May 22, 2013

pjedrzejewski May 22, 2013

stloyd May 22, 2013

norberttech May 22, 2013

pjedrzejewski commented May 22, 2013

pjedrzejewski commented May 22, 2013

piotrantosik commented May 22, 2013

stloyd May 22, 2013

piotrantosik May 22, 2013

pjedrzejewski commented May 23, 2013

stloyd May 23, 2013

piotrantosik commented May 31, 2013

pjedrzejewski May 31, 2013

piotrantosik May 31, 2013

pjedrzejewski commented May 31, 2013

Enable images for Taxonomy/Taxon #92

Enable images for Taxonomy/Taxon #92

Conversation

piotrantosik commented May 20, 2013

Choose a reason for hiding this comment

pjedrzejewski commented May 21, 2013

Choose a reason for hiding this comment

piotrantosik commented May 22, 2013

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

pjedrzejewski commented May 22, 2013

pjedrzejewski commented May 22, 2013

piotrantosik commented May 22, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjedrzejewski commented May 23, 2013

Choose a reason for hiding this comment

piotrantosik commented May 31, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjedrzejewski commented May 31, 2013