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

Multiple channels with php-cli is not possible #9987

Closed
Prometee opened this issue Nov 30, 2018 · 15 comments
Closed

Multiple channels with php-cli is not possible #9987

Prometee opened this issue Nov 30, 2018 · 15 comments
Labels
Future Issues and PRs which are blocked by outside constraints.

Comments

@Prometee
Copy link
Contributor

Prometee commented Nov 30, 2018

Sylius versions affected: > 1.2 I suppose

Description
When having two channels and try to use a command witch use a ChannelContext, it throw a ChannelNotFoundException

Steps to reproduce
Two channels defined and trying to use ChannelContext while executing commands.

Possible Solution
Create a PhpCliChannelContext (priority: -256) that check if we are in a command line env and load a Channel witch can configured by service configuration (calls: [['setDefaultChannelCode', ['MY_DEFAULT_CHANNEL_CODE']]]) or by a global Sylius Channel configuration.
If the defaultChannelCode is not provided, we could fallback to the first channel we found.

@mamazu
Copy link
Member

mamazu commented Dec 1, 2018

When executing commands you don't have any frame of reference which channel to use. I would suggest that your command should have an argument that is called channel and then you just load the channel from the repository.

@Prometee
Copy link
Contributor Author

Prometee commented Dec 1, 2018

That's why I propose to add a special ChannelContext. Some plugins like BitBagCommerce/SyliusCmsPlugin use ChannelContext with fixture loading for exemple, but other plugins are maybe doing this also.

I could propose 2 solutions :
The first one could be to add a PhpCliChannelContext
The second one could be to update SingleChannelContext with a setter setDefaultChannelCode witch can be null to keep the default behavior of this Service.

@mamazu
Copy link
Member

mamazu commented Dec 1, 2018

I would suggest a different approach. Write a ChannelAwareTrait that defines a channel property and setChannel and getChannel then you can configure it right in the command when you use the trait.

@pamil pamil added the Help Wanted Issues needing help and clarification. label Dec 18, 2018
@stale
Copy link

stale bot commented Mar 18, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Issues and PRs with no recent activity, about to be closed soon. label Mar 18, 2019
@stale stale bot closed this as completed Mar 25, 2019
@kayue
Copy link
Contributor

kayue commented Dec 5, 2019

Any recommended solution?

@vvasiloi vvasiloi reopened this Jan 25, 2022
@stale stale bot removed the Stale Issues and PRs with no recent activity, about to be closed soon. label Jan 25, 2022
@vvasiloi
Copy link
Contributor

vvasiloi commented Jan 26, 2022

The solution @Prometee suggested should work.
I suggested something similar to @mickaellosiewicz not long ago and he confirmed it works.

It has the following elements:

  1. A ChannelAwareCommandInterface and an abstract ChannelAwareCommand (or a ChannelAwareCommandTrait) implementing it, with a --sylius-channel=channel_code configured.
  2. A new channel context with a way to set the current channel and with the highest priority that will return a channel if set.
  3. A listener/subscriber that will listen to ConsoleEvents::COMMAND event and if the command implements ChannelAwareCommandInterface and sylius-channel option is set, then use it's value to set the current channel using the new context.
    Not sure if it's necessary to listen to ConsoleEvents::TERMINATE event too and remove the channel from the context.
    This will allow to easily make any command channel aware.
    In addition, the context can be reused to forcefully set the channel in other circumstance.

@lchrusciel
Copy link
Member

Hey folks!

I would like to add my 2 cents here. Contexts by design should be considered as stateful services. They reference to session, current request etc. As a result they should be used mostly in HTTP Controllers or as close as possible to them.

As a result all CLI and or MessengerHandlers should be context free. Still, this is a recommendation and not whole code is supporting this case, like related #13545.

From the ideological point of view, we even shouldn't be able to determine channel, locale, cart or customer of CLI operation. Perhaps, we should do some high-level early return for all these contexts to not execute services below, as they may be heavy.

This doesn't mean that a context where you may define default context may be useful.

@vvasiloi
Copy link
Contributor

vvasiloi commented Jan 27, 2022

@lchrusciel for that to work, Sylius would need to separate all of it's business logic from the HTTP layer and get the channel as an argument instead of relying on channel context everywhere. Respectively, the channel context should only be used in the HTTP layer to get the current channel and pass it downstream to the application's business layer.
The road to achieve that is long... probably even Sylius 3 long.
Right now most of the Sylius business logic is extensively coupled to the HTTP layer, most of it is inherited from the resource layer.
The most representative example is that you can't do anything in the resource layer without the infamous RequestConfiguration.
What @Prometee and I proposed is just a workaround until the foundation allows for better solutions.

@lchrusciel
Copy link
Member

Yes, you are correct - but I wanted to share generic perspective for them. I didn't say, that we didn't make any mistake or misuse with them. But the truth is, that they are stateful and therefore can make a lot of problems. Perhaps, we should just provide good default values for CLI and other not request based executions.

@vladfilimon
Copy link

vladfilimon commented Sep 7, 2022

This issue becomes more painful with Sylius Plus, when the website has multiple channels and you have a cli command that needs to check inventory of products for whatever reason. With Sylius Plus, one of the default filters when resolving inventory sources is Sylius\Plus\Inventory\Application\Filter\EnabledChannelInventorySourcesFilter. Instead of throwing an ChannelNotFoundException, it will catch it and simply return the inventorySource it received as argument. As in CLI I kept getting inventory sources that are not associated to the channels I was working, this took me some time to figure out..

@Rafikooo
Copy link
Contributor

Hi @vladfilimon, indeed, it is as you said. We'll propose a solution in the future :)

@jakubtobiasz jakubtobiasz added Future Issues and PRs which are blocked by outside constraints. and removed Help Wanted Issues needing help and clarification. labels Oct 6, 2022
@diimpp
Copy link
Member

diimpp commented Nov 18, 2022

I strongly concur with stateless approach, channel, currency, locale, user shouldn't be available in CLI as context.

Even if we're to skip messenger point and stateless as default approach, It's not uncommon to have background cli commands, which are specific to certain channels and which cannot accept any other channel as interchangeable. i.e. different channels/currencies/locales/users to different services, instead of one global channel/currency/locale/user to all services.

But if there is a big demand for this feature and not just erroneous usage of existing contexts in CLI, then maybe new set of Channel/Currency/Locale/User aware interfaces and provider interface could be done as separate system to context.

@vvasiloi
Copy link
Contributor

@diimpp I also agree that it should be stateless, but right now it's not possible as the stateful contexts are used all other the place in Sylius.
Even if the execution starts in a cli command, most likely down the execution patch there will be at least one Sylius service that uses a context.
Changing that requires a massive refactoring with tons of BC breaks.

@diimpp
Copy link
Member

diimpp commented Nov 18, 2022

@vvasiloi While Sylius "sync" and stateful nature is undeniable, I was working with shop-api based ecommerce for 3 years now with extensive modifications and I can't say I ever encountered context related issues in such stateless and async environment.

It would be good to document at least several such context-bottleneck places and devise solution based on practical needs. If refactor is too much and project was at acceptable level for all those years, then maybe this a issue for Sylius V2 to solve.

coldic3 added a commit that referenced this issue Dec 22, 2022
…onsole commands (jakubtobiasz)

This PR was merged into the 1.13 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.13 <!-- see the comment below -->                  |
| Bug fix?        | no                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no <!-- don't forget to update the UPGRADE-*.md file --> |
| Related tickets | related to #9987 and #14549                      |
| License         | MIT                                                          |

If you need a context, read Łukasz's comment in #14549.

Preview:
![CleanShot 2022-12-16 at 17 42 13@2x](https://user-images.githubusercontent.com/80641364/208146910-89e99e04-d33a-488c-8725-69cc74353f3b.png)


Commits
-------

eccb7dd Add a cookbook about dealing with multiple channels in console commands
@Rafikooo
Copy link
Contributor

Rafikooo commented Jan 4, 2023

Summary:
We don't want to merge it, as no Sylius CLI command is suffering from this problem. What is more, we do not consider such usage as a good solution to the problem, so we don't want to encourage others to solve it this way.

The cookbook:
https://docs.sylius.com/en/1.13/cookbook/cli/handle-multiple-channels-in-cli.html

@Rafikooo Rafikooo closed this as completed Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Future Issues and PRs which are blocked by outside constraints.
Projects
None yet
Development

No branches or pull requests

10 participants