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

Documentation issue: Unable to override statemachine (eg. remove state/transition) #12788

Open
leflings opened this issue Jul 7, 2021 · 21 comments

Comments

@leflings
Copy link

leflings commented Jul 7, 2021

Sylius docs version: 1.10 (though it looks unchanged for the last several versions). Problem was the same on 1.9. With 1.8 it worked.

Edit: provided reproduction that it works in 1.8

Description
I am trying to follow the guide for customizing state machines, specifically the part about how to remove a state and its transitions. This links to the cookbook about How to customize Sylius checkout which I am aware is outdated.

I am hoping, that since the possibility of removing states/transitions is hinted in the up-to-date part of the docs, some helpful eyes can take a look at this. Prior to 1.9 this approach seemed to work.

I have provided a repository reproducing the problem. Base commit is sylius/sylius-standard installation. Second commit is adding the src/Resources/SyliusCoreBundle/config/app/state_machine/sylius_order_checkout.yml file (unmodified, verbatim copy of the vendor file). Third commit removes two transitions.

Prior to any modification the output of bin/console debug:config winzou_state_machine sylius_order_checkout looks like this (full dump)

...

transitions:
    address:
        from:
            - cart
            - addressed
            - shipping_selected
            - shipping_skipped
            - payment_selected
            - payment_skipped
        to: addressed
    ...
...

After making changes to the local resource file I expect the output to reflect the changes like this:

...
transitions:
    address:
        from:
            - cart
            - addressed
            - shipping_selected
            - shipping_skipped
        to: addressed
    ...
...

However the two removed transitions ([payment_selected, payment_skipped] -> address) are still there.

This stopped working with 1.9. It worked with 1.8.

I am aware that with 1.9 Symfony was updated to 5.X, and I believe this somehow changed the behaviour of resource overriding, though I can't find anything about it in release notes. I have tried getting help for this in both the Sylius and Symfony slack channels, but with no luck.

I am hoping someone can help with this (and update the documentation - I'd be happy to help if only I knew what had changed). It is a very important usecase for us to remove states/transitions.

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

Can you do the same with 1.8 and confirm that it works?

@leflings
Copy link
Author

leflings commented Jul 7, 2021

I can

Reproduction repository for 1.8

Same methodology as in issue (modification)

Result of bin/console debug:config winzou_state_machine sylius_order_checkout before and after

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

Try config/packages/_sylius.yaml like in the docs.

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

I use to add them to config/state_machine/ and import them in config/state_machine.yaml.
For example the sylius_order_checkout state machine will be in config/state_machine/sylius_order_checkout.yaml and the config/state_machine.yaml will import it:

imports:
    - { resource: '../state_machine/sylius_order_checkout.yaml' }

@leflings
Copy link
Author

leflings commented Jul 7, 2021

I have tried both of those approaches, but they don't work. Problem is, as soon as they are provided "regularly" under config/ all yaml nodes are merged, so you can't remove things, only add or override "end nodes"

@leflings
Copy link
Author

leflings commented Jul 7, 2021

I don't think this file is loaded: https://github.com/leflings/sylius_statemachine/blob/master/src/Resources/SyliusCoreBundle/config/app/state_machine/sylius_order_checkout.yml

Yeah, it would seem like that is the case. Why, I don't get though. It would seem to be a change in Symfony 5, but I can't find anything in their documentation or release notes concerning this.

From the Sylius docs:

Open the CoreBundle/Resources/config/app/state_machine/sylius_order_checkout.yml and place its content in the src/Resources/SyliusCoreBundle/config/app/state_machine/sylius_order_checkout.yml which is a standard procedure of overriding configs in Symfony. Remove the shipping_selected and shipping_skipped states, select_shipping and skip_shipping transitions. Remove the select_shipping and skip_shipping transition from the sylius_process_cart callback.

I'm aware that this might be 'purely' a Symfony issue, but I've had no luck seeking help in their channels. My hope is that the Sylius community can help solve this, and make this usecase possible again,.

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

It's a change back from Symfony 4, when the directory structure changed and the application config files were no longer located in src/Resources. It was implemented in Sylius 1.3, by removing the AppKernel which was extending CoreBundle Kernel which loads config from Resources.
Unfortunately, these changes were not reflected in Sylius Standard, which didn't extend any of those kernels and was still loading config from Resources up to 1.8.9: https://github.com/Sylius/Sylius-Standard/blob/1.8.9/src/Kernel.php#L92-L105

@leflings
Copy link
Author

leflings commented Jul 7, 2021

Ah, that is a very helpful bit of detective work. Thank you!

So in this particular case it seems the problem can be solved by re-adding the override of getContainerLoader in src/Kernel.php.
Before it was removed it looked almost the same as https://github.com/symfony/symfony/blob/5.3/src/Symfony/Component/HttpKernel/Kernel.php#L757-L772 except for the added path (src/Resources) in the constructor call to FileLocator

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

Yes, that function was removed recently from both Sylius and Sylius-Standard, as part of the upgrade to Symfony 5: https://github.com/Sylius/Sylius/pull/12148/files#diff-9f0fabe23af928a9571327c664ebe4f50be0b60646e10e59ed40a30fdadd2532

That's what made it work before, because that code introduce in 1.3 allowed to override any config file (meaning replace it entirely).

Right now, without that code, you can't remove the transitions though configuration because the configuration definition does not have ->performNoDeepMerging() on the transitions node.

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

Here's a relevant thread: winzou/StateMachineBundle#32
This comment shows how to disable a state thanks to a special handling in the container extension, but it doesn't work for transitions.

@leflings
Copy link
Author

leflings commented Jul 7, 2021

Here's a relevant thread: winzou/StateMachineBundle#32
This comment shows how to disable a state thanks to a special handling in the container extension, but it doesn't work for transitions.

Yeah, I remember coming across that issue, when I was first trying to modify the statemachine.

Yes, that function was removed recently from both Sylius and Sylius-Standard, as part of the upgrade to Symfony 5: https://github.com/Sylius/Sylius/pull/12148/files#diff-9f0fabe23af928a9571327c664ebe4f50be0b60646e10e59ed40a30fdadd2532

That's what made it work before, because that code introduce in 1.3 allowed to override any config file (meaning replace it entirely).

Right now, without that code, you can't remove the transitions though configuration because the configuration definition does not have ->performNoDeepMerging() on the transitions node.

I like the approach of overriding the config files, replacing them entirely. Makes it easier to diff against the reference version whenever a new Sylius version comes along.

I guess this issue can be mark as resolved. Thanks again for the help @vvasiloi !

Perhaps the documentation should be updated with a link to this issue? I don't know if this would be an acceptable "solution" to the Sylius team

@leflings
Copy link
Author

leflings commented Jul 7, 2021

Ah, it seems it is not possible to just override getContainerLoader again, since the FileLocator no longer allows for providing custom paths.

I guess a solution would be to just make a compiler pass that alters the sm.configs container parameter.

@vvasiloi
Copy link
Contributor

vvasiloi commented Jul 7, 2021

I don't think this should be fixed in Sylius. The state machine bundle need a mechanism of removing transitions, just like it has for states.

@leflings
Copy link
Author

leflings commented Jul 8, 2021

I don't think there's anything for Sylius to fix either, except for maybe their documentation, to stop someone from scratching their head for too long. Maybe a link to this issue would be sufficient to help someone in the right direction, should they come across this scenario.

@leflings
Copy link
Author

leflings commented Jul 8, 2021

For anyone who comes across this issue, needing to completely modify state machines, as described in the docs, here's how I solved it with a compiler pass. Overriding files can be dumped in <projectDirectory>/state_machine_overrides (or anywhere - just modify the path in the constructor)

<?php

declare(strict_types=1);

namespace App\DependencyInjection\Compiler;

use Symfony\Component\Config\Definition\Processor;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\Finder\Finder;
use Symfony\Component\Finder\SplFileInfo;
use Symfony\Component\Yaml\Yaml;
use winzou\Bundle\StateMachineBundle\DependencyInjection\Configuration;
use winzou\Bundle\StateMachineBundle\DependencyInjection\winzouStateMachineExtension;

final class AlterStateMachineConfigurationPass implements CompilerPassInterface
{
    private string $overrideFilesDirectory;

    public function __construct(string $projectDir)
    {
        $this->overrideFilesDirectory = $projectDir . '/state_machine_overrides';
    }

    /** @psalm-suppress MixedAssignment, MixedArgument, PossiblyInvalidArgument, MixedArrayOffset */
    public function process(ContainerBuilder $container): void
    {
        /** @var array $configs */
        $configs = $container->getParameter('sm.configs');

        $finder = new Finder();
        $files = $finder->files()->in($this->overrideFilesDirectory);
        if (!$files->hasResults()) {
            return;
        }

        $parameterBag = $container->getParameterBag();
        $processor = new Processor();
        $configuration = new Configuration();
        $extension = new winzouStateMachineExtension();

        /** @var SplFileInfo $file */
        foreach ($files as $file) {
            $yaml = Yaml::parse($file->getContents());
            $yaml = $parameterBag->resolveValue($yaml);
            $stateMachines = $processor->processConfiguration($configuration, $yaml);
            $stateMachines = $extension->parseConfig($stateMachines);
            foreach ($stateMachines as $machineName => $machineDefinition) {
                if (array_key_exists($machineName, $configs)) {
                    $configs[$machineName] = $machineDefinition;
                }
            }
        }

        $container->setParameter('sm.configs', $configs);
    }
}

@Rafikooo
Copy link
Contributor

Hello @leflings,

we would like to inform you that we have placed this in our backlog 💃

Greetings,
Rafał 🚀

@philwagn
Copy link

Hello @leflings ,

I need to override the state machine. Where do you put this AlterStateMachineConfigurationPass class ?

@Rafikooo Is the feature still in backlog?

Thank you

@leflings
Copy link
Author

Hello @leflings ,

I need to override the state machine. Where do you put this AlterStateMachineConfigurationPass class ?

@Rafikooo Is the feature still in backlog?

Thank you

@philwagn You register it as a compiler pass. So I would put the class somewhere in App/DependencyInjection (but you can put it anywhere - it's just a class implementing an interface) and then register the pass in the kernel's build function.

@FarahInventiv
Copy link

Hello @leflings !
I am still facing the same problem even after trying your solution, im using sylius 1.10 ?

@leflings
Copy link
Author

Hello @leflings ! I am still facing the same problem even after trying your solution, im using sylius 1.10 ?

Hi @FarahInventiv. What problem are you still facing exactly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants