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-30573: As a Developer I want to filter by status when using ContentService::loadVersions method #2652

Merged
merged 6 commits into from
Jun 3, 2019

Conversation

kmadejski
Copy link
Member

@kmadejski kmadejski commented May 21, 2019

Question Answer
JIRA issue EZP-30573
Improvement yes
New feature no
Target version 7.5/master
BC breaks no
Tests pass yes
Doc needed no

This PR introduces the new $status argument for ContentService::loadVersions method.
For more information please refer to JIRA ticket.

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.

This is a feature, not an improvement, moreover it changes API interface for 3 bug-fix LTS releases if merged into 6.7. Please target it to master.

@kmadejski
Copy link
Member Author

kmadejski commented May 21, 2019

@alongosz I totally understand your point of view, even agree with it, but my motivation for this PR is a customer's report complaining about the performance of CleanupVersionsCommand.
This change should help improve it significantly, I believe.

Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

Besides inline comments, the integration test is missing for introduced changes

*
* @return \eZ\Publish\API\Repository\Values\Content\VersionInfo[] Sorted by creation date
*/
public function loadVersions(ContentInfo $contentInfo)
public function loadVersions(ContentInfo $contentInfo, $status = null)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering about introducing struct representing "loading options" here: In the feature, we might need to support other filters (e.g. creator) and pagination

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. It would be nice to have something like that, but honestly, I really don't want to do anything more than what is necessary for improving the mentioned command.
Good idea, but it should go to master then 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

And we don't currently do struct for loading options, we did for a bit on load content, but we dropped it when we found a way around.

@kmadejski
Copy link
Member Author

@adamwojs I added integration tests as you requested in: 9924af6.

@kmadejski kmadejski changed the base branch from 6.7 to 7.5 May 29, 2019 13:56
@kmadejski
Copy link
Member Author

@alongosz I rebased it against 7.5 as we discussed

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.

I've missed one more important thing:

eZ/Publish/API/Repository/ContentService.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/ContentService.php Outdated Show resolved Hide resolved
@kmadejski
Copy link
Member Author

@alongosz / @adamwojs / @andrerom all requested changes have been added, tests are green too. Could you please review again?

@kmadejski kmadejski requested a review from ViniTou May 31, 2019 08:30
*
* @param \eZ\Publish\API\Repository\Values\Content\ContentInfo $contentInfo
* @param null|int $status
Copy link
Contributor

@ViniTou ViniTou May 31, 2019

Choose a reason for hiding this comment

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

nitpick : int|null,
this is covered by new cs-fixer afair.

This have multiple occurrence, btw.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 25fb74f.

@lserwatka
Copy link
Member

@micszo we need round of QA here.

@micszo micszo self-assigned this Jun 3, 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 EE v2.5.1 with diff.

@micszo micszo removed their assignment Jun 3, 2019
@lserwatka lserwatka merged commit a168ba7 into ezsystems:7.5 Jun 3, 2019
@alongosz alongosz deleted the ezp_30573_version_status branch June 3, 2019 12:54
@alongosz
Copy link
Member

alongosz commented Jun 3, 2019

Merged up to master as well.

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