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

Upgrade rest bundle #167

Merged
merged 16 commits into from
Jun 25, 2020
Merged

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Jun 4, 2020

Q A
Bug fix? no
New feature? no
BC breaks? not anymore
Deprecations? no
Related tickets
License MIT

@loic425 loic425 requested a review from a team as a code owner June 4, 2020 18:45
@loic425
Copy link
Member Author

loic425 commented Jun 4, 2020

@pamil we have a lot of work here 🤣

@loic425
Copy link
Member Author

loic425 commented Jun 9, 2020

@lchrusciel @pamil
I worked a lot today on this pull request. Tell me if I am in the good direction or not.

src/Bundle/Controller/Parameters.php Outdated Show resolved Hide resolved
src/Bundle/Controller/ViewHandler.php Show resolved Hide resolved
src/Bundle/test/app/config/config.yml Outdated Show resolved Hide resolved
@loic425
Copy link
Member Author

loic425 commented Jun 10, 2020

@pamil To copy here what we said in slack.

We can create a recipe for this bundle with this configuration:

fos_rest:
    body_listener:
        enabled: true

UPGRADE-1.7.md Outdated Show resolved Hide resolved
UPGRADE-1.7.md Outdated Show resolved Hide resolved
src/Bundle/Controller/ViewHandler.php Show resolved Hide resolved
src/Bundle/test/app/config/config.yml Outdated Show resolved Hide resolved
loic425 and others added 4 commits June 17, 2020 16:59
Co-authored-by: Kamil Kokot <kamil@kokot.me>
Co-authored-by: Kamil Kokot <kamil@kokot.me>
…Handler.php

Co-authored-by: Kamil Kokot <kamil@kokot.me>
@loic425 loic425 force-pushed the feature/upgrade-rest-bundle branch from 32279f1 to 2b0c45f Compare June 17, 2020 15:40
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

looks good to me

@@ -58,7 +58,8 @@ public function get(RequestConfiguration $requestConfiguration, RepositoryInterf
$requestConfiguration->getPaginationMaxPerPage(),
$paginationLimits
));
$paginator->setCurrentPage($request->query->get('page', 1));
$currentPage = (int) $request->query->get('page', '1');
Copy link
Member

Choose a reason for hiding this comment

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

Woulndn't it be better?

Suggested change
$currentPage = (int) $request->query->get('page', '1');
$currentPage = (int) $request->query->getInt('page', '1');

Copy link
Member Author

Choose a reason for hiding this comment

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

@lchrusciel Yes, but, if it returns an integer, we don't need the (int) conversion after :)

Copy link
Member Author

@loic425 loic425 Jun 23, 2020

Choose a reason for hiding this comment

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

I pushed another suggestion to remove the unnecessary int conversion in your suggestion.

@@ -58,7 +58,8 @@ public function get(RequestConfiguration $requestConfiguration, RepositoryInterf
$requestConfiguration->getPaginationMaxPerPage(),
$paginationLimits
));
$paginator->setCurrentPage($request->query->get('page', 1));
$currentPage = (int) $request->query->get('page', '1');
Copy link
Member Author

Choose a reason for hiding this comment

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

@lchrusciel

Suggested change
$currentPage = (int) $request->query->get('page', '1');
$currentPage = $request->query->getInt('page', '1');

@pamil pamil merged commit 9196919 into Sylius:master Jun 25, 2020
@pamil
Copy link
Contributor

pamil commented Jun 25, 2020

Thank you, Loïc! 🎉

@loic425 loic425 deleted the feature/upgrade-rest-bundle branch June 25, 2020 13:23
@esserj esserj mentioned this pull request Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants