Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Product images deletion fix #9906

Merged
merged 4 commits into from
Nov 12, 2018

Conversation

Zales0123
Copy link
Member

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

Before

The problem was, whenever product deletion failed (for example because the variant is already used in a placed order), images files were still removed. The reason for that was, that the ImagesRemoveListener were listening on postRemove doctrine event, which is called even if flush ends with an error.

Proposed solution

I've refactored ImagesRemoveListener to listen on both onFlush and postFlush events. It saves paths from scheduled to remove entities with ImageInterface type and removes them with ImageUploader and CacheManager when the whole transaction passes (is there any BC Break? cc @pamil).

Test

During the implementation, I've encountered some problems with images assertions in Behat tests. The problem was, the images files in the cache were not generated while visiting them with mink driver in Behat page. I propose a new service ImageExistenceChecker, which is based on checking the existence of a file, rather than visiting checked file URL (which I find a little bit more appropriate). I'm not sure about the implementation - I suppose I could have made too many assumptions about the folders structure (parameter %__symfony__.kernel.project_dir%/web/ should be for sure changed in Sylius 1.3+) and I would be grateful for any ideas how this service can be improved/simplified.

Nonetheless, it works and fixes a bug, which is definitely a good thing 😄 🐃

@Zales0123 Zales0123 added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Nov 7, 2018
@Zales0123 Zales0123 requested a review from a team November 7, 2018 18:26
@Zales0123 Zales0123 changed the base branch from master to 1.2 November 7, 2018 18:27
Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

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

I guess that it fixes #8899 and closes #8909

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants