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

IBX-296: Provided new way to handle archived versions #3110

Merged

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Jun 30, 2021

Question Answer
JIRA issue https://issues.ibexa.co/browse/IBX-296
Bug/Improvement yes
New feature yes
Target version 7.5
BC breaks yes/no
Tests pass yes
Doc needed yes

Fix is built from two separate approaches:

  1. Disable removing archived versions on publish by using remove_archived_versions_on_publish configuration. That can save some time based on how many versions are going to be removed. If someone knows what he is doing, that process can be moved to scheduled ezplatform:content:cleanup-versions command.

  2. There are a lot of expensive count calls with WHERE LIKE to ezimagefile table in \eZ\Publish\Core\FieldType\Image\ImageStorage\Gateway\DoctrineStorage::countImageReferences. As it seems there is no need to use LIKE in current case, new isImageReferenced method was introduced.

but:
3. I have no idea how to implement proper integration tests with changes to content service settings.

QA:

This needs testing in both areas:

  • removing archived versions over the limit + new configuration setting
  • removing images from filesystem that are no longer related to any content.

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@ViniTou ViniTou requested review from alongosz, Nattfarinn and a team June 30, 2021 15:36
@ViniTou ViniTou added the Doc needed The changes require some documentation label Jun 30, 2021
@Steveb-p Steveb-p requested a review from a team June 30, 2021 20:07
@ViniTou ViniTou requested a review from Steveb-p July 1, 2021 06:14
@Steveb-p Steveb-p self-requested a review July 1, 2021 06:39
@ViniTou ViniTou requested a review from a team July 1, 2021 08:01
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

I'm not sure if proper integration tests can be written without introducing a lot of additional code, so 👍 from me.

@ViniTou ViniTou requested a review from a team July 1, 2021 08:14
Copy link
Contributor

@Nattfarinn Nattfarinn left a comment

Choose a reason for hiding this comment

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

I would deprecate current countImageReferences method and provide a proper copy of it without like but eq. We know that our countImageReferences should never be used as it is flawed by design.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Indeed I don't see easy way to modify that specific config for tests. We need to think about that as a follow up.
One remark:

eZ/Publish/Core/FieldType/Image/ImageStorage/Gateway.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
@ViniTou ViniTou requested a review from alongosz July 1, 2021 11:03
@ViniTou ViniTou requested a review from Nattfarinn July 1, 2021 11:39
Copy link

@kacper-wieczorek-ibexa kacper-wieczorek-ibexa left a comment

Choose a reason for hiding this comment

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

QA Approved - tested on 2.5.21 and 3.3.5

@lserwatka lserwatka merged commit 203cb94 into ezsystems:7.5 Jul 7, 2021
@lserwatka
Copy link
Member

You can merge it up now.

@ViniTou ViniTou deleted the IBX-296-clearing-archived-version-timeouts branch July 7, 2021 13:27
@DominikaK DominikaK removed the Doc needed The changes require some documentation label Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants