-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
bartoszpietrzak1994
merged 4 commits into
Sylius:1.2
from
Zales0123:product-images-deletion-fix
Nov 12, 2018
Merged
Product images deletion fix #9906
bartoszpietrzak1994
merged 4 commits into
Sylius:1.2
from
Zales0123:product-images-deletion-fix
Nov 12, 2018
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zales0123
added
the
Potential Bug
Potential bugs or bugfixes, that needs to be reproduced.
label
Nov 7, 2018
Zales0123
force-pushed
the
product-images-deletion-fix
branch
from
November 8, 2018 08:26
1f3c663
to
3578967
Compare
pamil
suggested changes
Nov 9, 2018
Zales0123
force-pushed
the
product-images-deletion-fix
branch
from
November 12, 2018 08:33
3578967
to
8852676
Compare
pamil
approved these changes
Nov 12, 2018
lchrusciel
approved these changes
Nov 12, 2018
GSadee
approved these changes
Nov 12, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bartoszpietrzak1994
approved these changes
Nov 12, 2018
This was referenced Nov 12, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 onpostRemove
doctrine event, which is called even if flush ends with an error.Proposed solution
I've refactored
ImagesRemoveListener
to listen on bothonFlush
andpostFlush
events. It saves paths from scheduled to remove entities withImageInterface
type and removes them withImageUploader
andCacheManager
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 😄 🐃