Change all MasterRequest calls to MainRequest #13251

Roshyo · 2021-10-26T14:55:32Z

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

For now, it should accept both until Symfony <5.3 support is dropped

src/Sylius/Bundle/ApiBundle/spec/Context/TokenValueBasedCartContextSpec.php

src/Sylius/Bundle/ApiBundle/Context/TokenValueBasedCartContext.php

4c0n · 2021-10-27T08:27:46Z

src/Sylius/Bundle/AdminBundle/EventListener/ResourceDeleteSubscriber.php

+        if (\method_exists($event, 'isMainRequest')) {
+            $isMainRequest = $event->isMainRequest();
+        } else {
+            $isMainRequest = $event->isMasterRequest();


Maybe it would be good to add deprectation warnings to these clauses. Perhaps using:
https://github.com/symfony/deprecation-contracts/blob/main/function.php

What do you mean ? The idea here is just to have a version that will work on both Symfony 4.x and 5.3+ without deprecation warning. Then when dropping <5.3 support, only the mainRequest part will stay.

But I don't see the point in throwing deprecation notice when the goal is to hide them

Deprecation warnings are intended on classes or methods, not inside methods. Also it's well known¹ that the hundreds of deprecations being thrown these days because of this master => main rename is slowing down performance so please don't add one more :(

Footnotes

https://stackoverflow.com/a/1869185/4819591 ↩

Alright, well the point is to have some sort of pointer that this needs to be removed at some point, in the current situation it is not documented and will continue to work forever. Maybe it's an idea to add a comment or something then?
This may be a wild idea, but there is a lot of duplication of this logic now, perhaps introduce a separate class for this?

I also thought of a separate class, like RequestProvider or something. Because the logic is often the same, but with some small differences. But it seems way too much for this.

But I agree with you that we need to store somewhere to get rid of this once we only support 5.3+

Perhaps in some .md file ?

So you're suggesting to merge the current state first and then have it refactored into the new component in a different PR? Why exactly?
If it's too much work, perhaps I could lend a helping hand. 💪

Yes, that would be my proposition.

First of all, because creating a new component seems more than just one folder. Sylius is hosting them as separate packages.

Second thing would be that right now, this PR is really easy to read/understand. And I have the small hope that this way, it will get approved/merged before next release 😄

Valid points, unfortunately.

Hey folks, thanks for contribution and discussion about it. Sample trait could be created behind the https://github.com/SyliusLabs umbrella, but it may be too much. The problem is, that 1.11 branch has been already split. I'm not sure, it will land in Sylius v1.11.X series.

I think, we can add note to: #13274 or separate issue, as Youri mentioned

vvasiloi · 2022-01-03T18:09:20Z

I was almost sure this was already merged...

Edit: I also triggered a re-run of all checks, because one failed with a segmentation fault.

Roshyo · 2022-01-04T05:49:03Z

Yeah, me too 🤔 IDK what happened

vvasiloi · 2022-01-04T06:01:39Z

Looks like it didn't re-run the jobs...
Can you rebase and push, @Roshyo?

4c0n · 2022-01-04T11:44:18Z

So in the end there is no mention of having to remove the if statements at some point in the future anywhere?
I don't want to block merging this PR, but I do feel we need at least something as a reminder that this requires work when isMasterRequest() is finally fased out.

Could be a github issue or PR as well as far as I'm concerned.

Roshyo · 2022-01-04T11:51:19Z

I'm ok to write it somewhere, but I can't find any md file that would fit. Perhaps an issue is the way to go 🤔

lchrusciel · 2022-01-04T13:33:51Z

src/Sylius/Bundle/AdminBundle/EventListener/ResourceDeleteSubscriber.php

+        if (\method_exists($event, 'isMainRequest')) {
+            $isMainRequest = $event->isMainRequest();
+        } else {
+            $isMainRequest = $event->isMasterRequest();


Hey folks, thanks for contribution and discussion about it. Sample trait could be created behind the https://github.com/SyliusLabs umbrella, but it may be too much. The problem is, that 1.11 branch has been already split. I'm not sure, it will land in Sylius v1.11.X series.

src/Sylius/Bundle/AdminBundle/EventListener/ResourceDeleteSubscriber.php

lchrusciel · 2022-01-04T14:08:55Z

src/Sylius/Bundle/AdminBundle/EventListener/ResourceDeleteSubscriber.php

+        if (\method_exists($event, 'isMainRequest')) {
+            $isMainRequest = $event->isMainRequest();
+        } else {
+            $isMainRequest = $event->isMasterRequest();


I think, we can add note to: #13274 or separate issue, as Youri mentioned

lchrusciel

I meant, that we should place this comment above all occurrence of this ACL ifs. But thats a minor stuff. The plan will be as follows:

Merge it to 1.11, to get rid of deprecation message
Bump Symfony requirements to Sf ^5.4 on master
Remove this ACL from the master
At the end, trait would live with us too short, to consider it as separate package in my opinion.

src/Sylius/Bundle/ApiBundle/Context/TokenValueBasedCartContext.php

src/Sylius/Bundle/ApiBundle/spec/Context/TokenValueBasedCartContextSpec.php

src/Sylius/Component/Channel/spec/Context/CachedPerRequestChannelContextSpec.php

src/Sylius/Component/Channel/spec/Context/RequestBased/ChannelContextSpec.php

lchrusciel · 2022-01-05T14:34:34Z

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "main-request-change" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

lchrusciel · 2022-01-11T15:37:50Z

Thanks, Stephane! 🥇

Roshyo requested a review from a team as a code owner October 26, 2021 14:55

probot-autolabeler bot added Admin AdminBundle related issues and PRs. API APIs related issues and PRs. Shop ShopBundle related issues and PRs. labels Oct 26, 2021

vvasiloi reviewed Oct 26, 2021

View reviewed changes

src/Sylius/Bundle/ApiBundle/spec/Context/TokenValueBasedCartContextSpec.php Show resolved Hide resolved

loevgaard reviewed Oct 26, 2021

View reviewed changes

src/Sylius/Bundle/ApiBundle/Context/TokenValueBasedCartContext.php Outdated Show resolved Hide resolved

4c0n reviewed Oct 27, 2021

View reviewed changes

vvasiloi approved these changes Jan 3, 2022

View reviewed changes

Roshyo force-pushed the main-request-change branch from 501e8b0 to 4869756 Compare January 4, 2022 06:18

loic425 approved these changes Jan 4, 2022

View reviewed changes

stloyd approved these changes Jan 4, 2022

View reviewed changes

lchrusciel reviewed Jan 4, 2022

View reviewed changes

lchrusciel approved these changes Jan 5, 2022

View reviewed changes

lchrusciel changed the base branch from master to 1.11 January 5, 2022 14:34

lchrusciel force-pushed the main-request-change branch from 8bc849a to 1113fa8 Compare January 5, 2022 14:34

Roshyo force-pushed the main-request-change branch from 1113fa8 to 4869756 Compare January 6, 2022 07:05

lchrusciel force-pushed the main-request-change branch from 4869756 to 9354609 Compare January 11, 2022 15:01

Roshyo added 3 commits January 11, 2022 16:09

Change all MasterRequest calls to MainRequest

fe0aae8

Reverting removed php7 typing

98a6a04

Fix missing space

f78ef8b

lchrusciel force-pushed the main-request-change branch from 9354609 to f78ef8b Compare January 11, 2022 15:09

lchrusciel merged commit d016099 into Sylius:1.11 Jan 11, 2022

lchrusciel mentioned this pull request Jan 11, 2022

Remove ACL layer need for MasterRequest to MainRequest renaming #13470

Closed

2 tasks

Roshyo mentioned this pull request Jan 12, 2022

Symfony 6 support #13274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change all MasterRequest calls to MainRequest #13251

Change all MasterRequest calls to MainRequest #13251

Roshyo commented Oct 26, 2021

4c0n Oct 27, 2021

Roshyo Oct 27, 2021 •

edited

Loading

loevgaard Oct 27, 2021

4c0n Oct 27, 2021

Roshyo Oct 27, 2021

4c0n Oct 27, 2021

Roshyo Oct 27, 2021

4c0n Oct 27, 2021

lchrusciel Jan 4, 2022

lchrusciel Jan 4, 2022 •

edited

Loading

vvasiloi commented Jan 3, 2022 •

edited

Loading

Roshyo commented Jan 4, 2022

vvasiloi commented Jan 4, 2022 •

edited

Loading

4c0n commented Jan 4, 2022

Roshyo commented Jan 4, 2022

lchrusciel Jan 4, 2022

lchrusciel Jan 4, 2022 •

edited

Loading

lchrusciel left a comment

lchrusciel commented Jan 5, 2022

lchrusciel commented Jan 11, 2022

Change all MasterRequest calls to MainRequest #13251

Change all MasterRequest calls to MainRequest #13251

Conversation

Roshyo commented Oct 26, 2021

Choose a reason for hiding this comment

Roshyo Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchrusciel Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

vvasiloi commented Jan 3, 2022 • edited Loading

Roshyo commented Jan 4, 2022

vvasiloi commented Jan 4, 2022 • edited Loading

4c0n commented Jan 4, 2022

Roshyo commented Jan 4, 2022

Choose a reason for hiding this comment

lchrusciel Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

lchrusciel left a comment

Choose a reason for hiding this comment

lchrusciel commented Jan 5, 2022

lchrusciel commented Jan 11, 2022

Roshyo Oct 27, 2021 •

edited

Loading

lchrusciel Jan 4, 2022 •

edited

Loading

vvasiloi commented Jan 3, 2022 •

edited

Loading

vvasiloi commented Jan 4, 2022 •

edited

Loading

lchrusciel Jan 4, 2022 •

edited

Loading