-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
EZP-30857: Fixed assessment of image removal feasibility #3056
Merged
alongosz
merged 4 commits into
ezsystems:7.5
from
alongosz:fix-7.5/ezp-30857-remove-deleted-image-file
Aug 13, 2020
Merged
EZP-30857: Fixed assessment of image removal feasibility #3056
alongosz
merged 4 commits into
ezsystems:7.5
from
alongosz:fix-7.5/ezp-30857-remove-deleted-image-file
Aug 13, 2020
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
6 tasks
alongosz
force-pushed
the
fix-7.5/ezp-30857-remove-deleted-image-file
branch
from
August 6, 2020 17:06
3790eaa
to
41a0e43
Compare
5 tasks
alongosz
requested review from
adamwojs,
kaff,
mikadamczyk,
Nattfarinn,
ViniTou and
webhdx
August 6, 2020 19:10
ViniTou
approved these changes
Aug 7, 2020
alongosz
added a commit
to alongosz/ezplatform-kernel
that referenced
this pull request
Aug 7, 2020
Closed
7 tasks
mikadamczyk
approved these changes
Aug 12, 2020
mnocon
approved these changes
Aug 13, 2020
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.
Tested together with #3057
So, now the images are deleted from disk. But that will apply on future action of removing content. What about images that were not previously deleted ? |
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
7.5
,ezplatform-kernel:1.1
+When removing a Content item (or its Version) with a changed Image Field Type file, the file associated with a previous Version was not deleted from a file system.
This happened because relation of
ezimagefile
.contentobject_attribute_id
toezcontentobject_attribute.id
does not contain Version number (attribute - a.k.a. Field - ID is the same for every Version).One of the approaches would be to introduce versioning for
ezimagefile
(#2835). This however always causes issues due to BC break on schema.Instead, I'm proposing an alternative - change of assessment if image can be removed based on
ezimage
data_text
XML, which contains proper URI for a given Version. Instead of getting acount
against a product ofezimagefile
andezcontentobject_attribute
, data is fetched and processed to find the actual file system file URI for a given Version.The count cannot work because the resulting Cartesian product (slice/intersection of data) is incorrect - joins file paths for all Versions with a Field. The join itself is kept to make sure we're joining existing
ezimage
entry (7dca764).Additionally, I've discovered that image variations (aliases) are not removed at all. It didn't work because a produced file path was in the form of
../var/site/storage/images/_aliases/medium/var/site/storage/images/...
.Fixed it by applying removal service on a relative original binay file identifier (41a0e43).
I've discovered two other CS and DX issues, but for now I'll make follow-up PRs. Moreover we deal here with a responsibility mixup - Doctrine Gateway should not process XML but just return data. That however is a candidate for
ezplatform-kernel:master
- see ezsystems/ezplatform-kernel#96.QA
Note: Physical files can be found (and should not be found after deleting) by the following command:
TODO:
$ composer fix-cs
).