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

Use camel case for arguments and variables at CRUDController::historyCompareRevisionsAction() #6913

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

phansys
Copy link
Member

@phansys phansys commented Mar 5, 2021

Subject

Use camel case for arguments and variables at CRUDController::historyCompareRevisionsAction().

I am targeting this branch, because these changes respect BC.

Changelog

### Added
- Route parameters "baseRevision" and "compareRevision" for `CRUDController::historyCompareRevisionsAction()`.

### Deprecated
- Route parameters "base_revision" and "compare_revision" for `CRUDController::historyCompareRevisionsAction()`.

@phansys phansys marked this pull request as ready for review March 5, 2021 14:32
@phansys phansys requested a review from a team March 5, 2021 14:33
@phansys
Copy link
Member Author

phansys commented Mar 5, 2021

Tests are passing, but I think the exit code is not 0 given the limit configured for the deprecation messages:

OK, but incomplete, skipped, or risky tests!
Tests: 2248, Assertions: 5998, Skipped: 14.

@VincentLanglet
Copy link
Member

I think #6911 could fix your issue

@phansys phansys changed the title Use pascal case for arguments and variables at CRUDController::historyCompareRevisionsAction() Use camel case for arguments and variables at CRUDController::historyCompareRevisionsAction() Mar 5, 2021
@phansys phansys force-pushed the camel_case branch 2 times, most recently from 454a551 to 9eeda0a Compare March 5, 2021 15:54
@phansys phansys requested a review from VincentLanglet March 5, 2021 17:01
VincentLanglet
VincentLanglet previously approved these changes Mar 6, 2021
@VincentLanglet VincentLanglet requested a review from a team March 6, 2021 13:46
@@ -47,7 +47,7 @@ public function build(AdminInterface $admin, RouteCollection $collection)
if ($this->manager->hasReader($admin->getClass())) {
$collection->add('history', sprintf('%s/history', $admin->getRouterIdParameter()));
$collection->add('history_view_revision', sprintf('%s/history/{revision}/view', $admin->getRouterIdParameter()));
$collection->add('history_compare_revisions', sprintf('%s/history/{base_revision}/{compare_revision}/compare', $admin->getRouterIdParameter()));
$collection->add('history_compare_revisions', sprintf('%s/history/{baseRevision}/{compareRevision}/compare', $admin->getRouterIdParameter()));
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you should change the route in a stable release

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'll try to check how could we deal with this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@core23, I added a BC layer for those cases where the param names are explicitly used.

@phansys phansys force-pushed the camel_case branch 2 times, most recently from 752b437 to 301eefc Compare March 9, 2021 20:32
@phansys phansys requested review from core23, VincentLanglet and a team March 9, 2021 20:33
@phansys phansys added minor and removed pedantic labels Mar 9, 2021
VincentLanglet
VincentLanglet previously approved these changes Mar 9, 2021
@phansys phansys requested a review from a team March 9, 2021 21:38
@@ -866,38 +866,62 @@ public function historyCompareRevisionsAction($id = null, $base_revision = null,

$reader = $manager->getReader($this->admin->getClass());

// NEXT_MAJOR: Remove this condition.
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide a test that verifies this hack?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@phansys phansys requested a review from a team March 10, 2021 09:48
@phansys phansys requested a review from a team March 10, 2021 10:21
@wbloszyk wbloszyk requested a review from a team March 10, 2021 10:43
@VincentLanglet VincentLanglet merged commit 23ef0c9 into sonata-project:3.x Mar 10, 2021
@VincentLanglet
Copy link
Member

Thanks @phansys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants