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

EZP-30729: Added a possibility to exclude Content Types when running CleanupVersionsCommand #2691

Merged
merged 7 commits into from
Aug 21, 2019

Conversation

kmadejski
Copy link
Member

Question Answer
JIRA issue EZP-30729
Bug yes
New feature no
Target version 6.7/6.13/7.5/master
BC breaks yes
Tests pass yes
Doc needed yes

This PR adds a possibility to exclude particular ContentTypes of which versions should not be removed. This option always includes content objects of default User ContentType (ID: 4, unfortunately, there is no const anywhere to use it instead of hardcoding it).

Moreover, I've added a ProgressBar which indicates the progress. In some cases, it takes some time to clean up a huge number of versions so the user might have a wrong impression that nothing is going on.

A second PR, introducing the usage of $status in ContentService::loadVersions method (introduced in: #2652) will be done soon, against 7.5 only.

TODO:

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

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.

Issues:

  1. I don't really follow the change of how verbosity behaves from the default way (see the diff comments), for me it would be rather unexpected.
  2. I'm having trouble running it with the default value of --keep. AFAICS neither options.default_version_archive_limit nor default_version_archive_limit is mapped to dynamic settings configuration. It's just a repository option (there's RepositoryConfigurationProvider AFAIR for that). But maybe I'm missing something? I tried to run it w/o any parameters and got:
[eZ\Publish\Core\MVC\Exception\ParameterNotFoundException]                               
  Parameter 'options.default_version_archive_limit' with namespace '' could not be found.
  1. I see an issue with processing the new parameter (see the diff comments).

@andrerom
Copy link
Contributor

andrerom commented Jul 9, 2019

In ContentService->internalPublishVersion() we have for quite some time been deleting overflow versions on publish. So it would be good if we could come to the bottom of why they have this issue, as this only workarounds part of the features which deals with deleting versions.

That said, including and excluding content types seems like a very good improvement on the command regardlessly. So comment above is only about setting user as default as opposed to better understanding what goes wrong on version deletion of users (users have extra tables, maybe that is part of the issue here, ...).

@alongosz
Copy link
Member

alongosz commented Jul 9, 2019

I still have a question about:

  1. I'm having trouble running it with the default value of --keep. AFAICS neither options.default_version_archive_limit nor default_version_archive_limit is mapped to dynamic settings configuration. It's just a repository option (there's RepositoryConfigurationProvider AFAIR for that). But maybe I'm missing something? I tried to run it w/o any parameters and got:
[eZ\Publish\Core\MVC\Exception\ParameterNotFoundException]                               
  Parameter 'options.default_version_archive_limit' with namespace '' could not be found.

Is it just me? :)

@kmadejski
Copy link
Member Author

I still have a question about...

Unfortunately, not only you. I'm looking into that 😉

@kmadejski
Copy link
Member Author

@alongosz issue no 2, @adamwojs removed usage of CT IDs and introduced the usage of identifiers, both done in 359251b

@alongosz alongosz changed the title EZP-30729: CleanupVersionsCommand causes database issues when removing versions of User ContentType EZP-30729: Added a possibility to exclude Content Types when running CleanupVersionsCommand Jul 9, 2019
@micszo micszo self-assigned this Aug 8, 2019
Copy link
Member

@micszo micszo 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 on eZ Platform v1.7.9.
Testing other versions will be possible after merge / second PR is rebased.

@micszo micszo removed their assignment Aug 21, 2019
@alongosz alongosz merged commit 91a24d7 into ezsystems:6.7 Aug 21, 2019
@alongosz alongosz deleted the ezp_30729 branch August 21, 2019 14:12
@alongosz
Copy link
Member

@kmadejski could you merge up?

@kmadejski
Copy link
Member Author

Merged up in: e4ce240 / 5b6db1c / df5ff8d.

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.

6 participants