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

Get quantity from request as an integer. #239

Merged
merged 5 commits into from
Nov 29, 2017

Conversation

ce-bo
Copy link

@ce-bo ce-bo commented Nov 9, 2017

PR does fix this issue: #235

@lchrusciel
Copy link
Member

Hey @ce-bo. Thanks for contribution. I would merge it but the tests are failing :( Can you adjust them as well?

@ce-bo
Copy link
Author

ce-bo commented Nov 14, 2017

Hi @lchrusciel, the tests have passed now. Could you merge it please?

@@ -31,7 +31,7 @@ public function __construct(Request $request)
{
$this->token = $request->attributes->get('token');
$this->id = $request->attributes->get('id');
$this->quantity = $request->request->get('quantity');
$this->quantity = $request->request->has('quantity') ? $request->request->getInt('quantity') : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Quantity cannot be null, it's declared as int. $request->request->getInt('quantity', 1) instead?

Copy link
Member

Choose a reason for hiding this comment

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

$request->request->getInt('quantity', 0)? This should trigger validation error. Question is, what should be a default behaviour. 1 as a default would be more flexible for frontends...

Copy link
Contributor

Choose a reason for hiding this comment

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

If we allow not to pass quantity then it makes sense to use 1 as the default value. If we require quantity then we should throw an exception here if it does not exist.

Copy link
Author

Choose a reason for hiding this comment

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

The decision is up to you.

Copy link
Member

Choose a reason for hiding this comment

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

Let's add 1 as default. But we need to mention it in docs.

Copy link
Author

Choose a reason for hiding this comment

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

Can you adjust it, please?

Copy link
Member

Choose a reason for hiding this comment

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

sure, but it will take some time

@@ -44,7 +44,7 @@ public static function fromArray(array $item)

public static function fromRequest(Request $request)
{
return new self($request->attributes->get('token'), $request->request->get('productCode'), $request->request->get('options'), $request->request->get('quantity'));
return new self($request->attributes->get('token'), $request->request->get('productCode'), $request->request->get('options'), $request->request->has('quantity') ? $request->request->getInt('quantity') : null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here and so on.

@pjedrzejewski
Copy link
Member

Hi @ce-bo, do you plan to continue on this PR? :)

@ce-bo
Copy link
Author

ce-bo commented Nov 28, 2017

Hi @pjedrzejewski, unfortunately not. I already discussed with @lchrusciel that he would rather do it.

@lchrusciel lchrusciel merged commit cf1aabd into Sylius:master Nov 29, 2017
@lchrusciel
Copy link
Member

Thanks @ce-bo for all your effort! And welcome to our community! 🎉

@ce-bo
Copy link
Author

ce-bo commented Nov 29, 2017

@lchrusciel thanks for finalizing!

@ce-bo ce-bo deleted the quantity-as-integer branch November 29, 2017 09:02
@lchrusciel
Copy link
Member

I will make a release this week

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.

4 participants