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

Cast employeeId to int in the bulk update employee command #16307

Merged
merged 1 commit into from Dec 16, 2019

Conversation

@atomiix
Copy link
Contributor

atomiix commented Nov 7, 2019

Questions Answers
Branch? develop
Description? This force EmployeeId constructor parameter to be an int
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #16246
How to test? Go to Configure > Advanced Parameters > Team > Employees page and try a bulk action

This change is Reviewable

@atomiix atomiix requested a review from PrestaShop/prestashop-core-developers as a code owner Nov 7, 2019
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Nov 7, 2019

Ping @atomiix : Travis is broken in Unit Tests.

@atomiix atomiix force-pushed the atomiix:fix-employee-bulk branch from eb66dce to 8e183f2 Nov 7, 2019
@PierreRambaud PierreRambaud changed the title add int type of the constructor parameter Add int type of the constructor parameter Nov 7, 2019
Copy link
Contributor

PierreRambaud left a comment

You also need to type int the constructor of EmployeeId ValueObject 🤔

@atomiix

This comment has been minimized.

Copy link
Contributor Author

atomiix commented Nov 7, 2019

@PierreRambaud problem is if we type int the constructor of EmployeeId ValueObject, tests fail because they are testing that if we send a string to the constructor it should throw an exception but if the constructor is type int, the string will be casted to int and so no exception will be thrown.

To have an exception to be thrown, It would need to set declare(strict_type=1) in the EmployeeId ValueObject php file as well as in every php file that is instantiating an EmployeeId ValueObject.

We had a discussion with @eternoendless and decided that we should add declare(strict_type=1) only to new files. So, that's why I didn't type int the EmployeeId constructor.

What do you think?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Nov 7, 2019

Why not. I was more thinking about adding declare(strict_type=1) each time we want to use type hint :) But he's the boss 😅
But you need to rename your pull request, you're not adding an int type, you're casting in int

@PierreRambaud PierreRambaud dismissed their stale review Nov 7, 2019

Eveything has been said

@atomiix atomiix changed the title Add int type of the constructor parameter Cast employeeId to int in the bulk update employee command Nov 7, 2019
@PierreRambaud PierreRambaud added this to the 1.7.7.0 milestone Nov 12, 2019
@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Nov 13, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 13, 2019

Hi @atomiix ,

The bug still appears when trying to delete the employee (for enable/disable bulk actions, it is OK) :/

Thanks !

@atomiix atomiix force-pushed the atomiix:fix-employee-bulk branch from 8e183f2 to c4a4e58 Dec 4, 2019
@PierreRambaud PierreRambaud merged commit fb37952 into PrestaShop:develop Dec 16, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Dec 16, 2019

Thanks @atomiix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.