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.

Sign up for GitHub

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

Jump to bottom

Added getter/setter for getCouponCode() #2825

Merged
merged 2 commits into from
Jan 4, 2023
Merged

Added getter/setter for getCouponCode() #2825

merged 2 commits into from
Jan 4, 2023

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Dec 18, 2022 •
edited
Loading

Description (*)

@elidrissidev #2586 (comment)

escapeHtml phpdoc doesn't say it accepts nullable data, after all it doesn't make sense to escape null.

Respecting the PHPdocs will make it easier in the future to add strict type hinting to methods instead of making everything nullable.

What do you say?

Yes, it does make no sense to escape null, but currently it is done.

I'd prefer the method, that takes most effort. Adding getter/setter methods ...

Added first getter/setter for test ... should make some changes to templates obsolete.

Related Pull Requests

  1. See [PHP 8.1] Fix passing null to non-nullable internal function params #2586 ... search for $this->getCouponCode()
  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@sreichel sreichel added the PHP 8.1 Related to PHP 8.1 label Dec 18, 2022
@github-actions github-actions bot added Component: Checkout Relates to Mage_Checkout Component: Sales Relates to Mage_Sales Template : base Relates to base template Template : rwd Relates to rwd template labels Dec 18, 2022
@sreichel sreichel changed the title [WIP] Added getter/setter [WIP] Added getter/setter (to avoid type casting/passing null) Dec 18, 2022
@fballiano fballiano marked this pull request as draft December 19, 2022 11:48
@fballiano
Copy link
Contributor

converted it to draft so that it doesn't appear in https://fabrizioballiano.com/om/pr_status.html ;-)

@sreichel
Copy link
Contributor Author

sreichel commented Dec 19, 2022 •
edited
Loading

This is ready to test ...

If its okay to fix it that way, ill add some commits.

@sreichel sreichel marked this pull request as ready for review December 19, 2022 21:31
@fballiano fballiano changed the title [WIP] Added getter/setter (to avoid type casting/passing null) Added getter/setter (to avoid type casting/passing null) Dec 20, 2022
ADDISON74
ADDISON74 approved these changes Dec 30, 2022
View reviewed changes
Copy link
Contributor

@ADDISON74 ADDISON74 left a comment

Choose a reason for hiding this comment

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

LGTM

fballiano
fballiano reviewed Dec 30, 2022
View reviewed changes
app/code/core/Mage/Sales/Model/Quote.php
* @param string $couponCode
* @return $this
*/
public function setCouponCode(string $couponCode) #static with php74
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "static with php74" comment mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When adding new methods i tried to add type hints and return types if possible ... , was not sure about adding self. Its a php8 change not 7.4 (https://php.watch/versions/8.0/static-return-type) ... comment can be removed.

However ... this should not be merged yet. If it's okay to add some getter/setter to avoid changes in templates, i'd also fix the other issues found by @elidrissidev.

@elidrissidev
Copy link
Member

I haven't checked this yet, usages of getCouponCode need to be checked carefully since this change will make it always a return a string even if not set.

@sreichel
Copy link
Contributor Author

Yes, but an empty string is no valid coupon code,

@elidrissidev
Copy link
Member

elidrissidev commented Dec 30, 2022 •
edited
Loading

I know, I meant just to make sure there's no code that expects null or something. I remember similar issue happened before due to introducing getters that change the type.

@sreichel
Copy link
Contributor Author

just to make sure there's no code that expects null or something

Think this would be found by phpstan ... https://phpstan.org/r/6b275cc5-439a-4af3-b201-b6c932a07b16

@elidrissidev
Copy link
Member

Just checked all the getCouponCode usages, the change looks safe to me.

@sreichel
Copy link
Contributor Author

Thanks for review. 👍

I'd add some more to fix the issues you've found during your test.

@sreichel
Copy link
Contributor Author

sreichel commented Jan 3, 2023

On hold ... #2884

@sreichel sreichel marked this pull request as draft January 3, 2023 03:22
@sreichel sreichel changed the title Added getter/setter (to avoid type casting/passing null) Added getter/setter for getCouponCode() (to avoid type casting/passing null) Jan 3, 2023
@sreichel sreichel changed the title Added getter/setter for getCouponCode() (to avoid type casting/passing null) Added getter/setter for getCouponCode() Jan 3, 2023
@sreichel
Copy link
Contributor Author

sreichel commented Jan 3, 2023

Just fix coupon code related code here.

@sreichel sreichel marked this pull request as ready for review January 3, 2023 23:47
Flyingmana
Flyingmana approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

good approach

@sreichel sreichel merged commit 81e11fa into OpenMage:1.9.4.x Jan 4, 2023
@sreichel sreichel deleted the php81/getter-setter branch January 4, 2023 00:32
fballiano pushed a commit that referenced this pull request Jan 4, 2023
@fballiano
Added getter/setter (#2825)
5a3f7e1
elidrissidev pushed a commit that referenced this pull request Jan 4, 2023
Use getCouponCode method instead of _getData(), ref #2825 (#2892)
9182521
fballiano pushed a commit that referenced this pull request Jan 4, 2023
@fballiano
Use getCouponCode method instead of _getData(), ref #2825 (#2892)
74bd26a
@elidrissidev elidrissidev mentioned this pull request Apr 10, 2023
Merged
@S0FTWEX S0FTWEX mentioned this pull request Aug 28, 2023
Closed
@elidrissidev elidrissidev mentioned this pull request Sep 4, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fballiano fballiano fballiano left review comments

@Flyingmana Flyingmana Flyingmana approved these changes

@ADDISON74 ADDISON74 ADDISON74 approved these changes

@elidrissidev elidrissidev Awaiting requested review from elidrissidev

Assignees
No one assigned
Labels
Component: Checkout Relates to Mage_Checkout Component: Sales Relates to Mage_Sales PHP 8.1 Related to PHP 8.1 Template : base Relates to base template Template : rwd Relates to rwd template
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sreichel @fballiano @elidrissidev @Flyingmana @ADDISON74