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

[Adjustment] Inject adjustment types that shall be cleared #9308

Merged
merged 2 commits into from
Apr 10, 2018

Conversation

Zales0123
Copy link
Member

@Zales0123 Zales0123 commented Apr 6, 2018

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Due to the fact, that cleared adjustments types has been set as static array, after adding new adjustment type (that is also processed during checkout), it was necessary to implement whole new class (with very similar/the same implementation, but different static array) and override the service definition. With this change, only definition overriding is needed.

@Zales0123 Zales0123 added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Apr 6, 2018
@pamil pamil added this to the 1.2 milestone Apr 6, 2018
@pamil
Copy link
Contributor

pamil commented Apr 6, 2018

Can't see any BC breaks, where are they?

@Zales0123
Copy link
Member Author

My bad, they've just vanished magically 😄 ⭐️

Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

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

Okay, one possible BC break here: if there are no arguments passed (new OrderAdjustmentsClearer()), we should use the default values.

@pamil
Copy link
Contributor

pamil commented Apr 6, 2018

IIRC we can get this info by calling func_num_args(), it will be 0 if there are no arguments passed.

function foo(array $arg = []) {
  var_dump(func_num_args());
}

foo(); // 0
foo([]); // 1

We also need to trigger user deprecation error there, just as Symfony does it:

@trigger_error('Pass adjustment types explicitly, ok?', E_USER_DEPRECATED);

@@ -26,6 +26,18 @@

public function __construct(array $adjustmentsToRemove = [])
{
if (0 === func_num_args()) {
@trigger_error('Adjustment types should be passed explicitly', E_USER_DEPRECATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

We also might want to mention that not passing them is deprecated since 1.2 and will be prohibited by 2.0

@Zales0123 Zales0123 force-pushed the inject-adjustments-to-clear branch from dec7ed2 to e7f020e Compare April 6, 2018 12:53
@pamil pamil merged commit 5ae406a into Sylius:master Apr 10, 2018
@pamil
Copy link
Contributor

pamil commented Apr 10, 2018

Yay, first deprecation in Sylius, thank you Mateusz! 🥇

@Zales0123
Copy link
Member Author

I'm very happy I could provide that, I hope we will be able to have as many deprecations as possible xD

@Zales0123 Zales0123 deleted the inject-adjustments-to-clear branch April 10, 2018 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants