[Maker] Resource, State provider and state processor makers #705

loic425 · 2023-04-14T15:10:22Z

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

Zales0123 · 2023-06-22T12:17:33Z

I suppose this one should be rebased with 1.11? cc @loic425

loic425 · 2023-06-23T08:54:47Z

I suppose this one should be rebased with 1.11? cc @loic425

@Zales0123 This is now ready to review :)
But this kind of features are not easy to review.

loic425 · 2023-09-01T10:41:37Z

src/Component/Tests/Symfony/Maker/MakeResourceTest.php

+            use Sylius\Component\Resource\Model\ResourceInterface;
+            use Symfony\Component\Uid\AbstractUid;
+
+            #[Resource]


Hum, I forgot to specify driver option to false.

loic425 · 2023-09-01T10:44:19Z

tests/Application/tmp/Sylius/State/BookItemProvider.php

+use Sylius\Component\Resource\Metadata\Operation;
+use Sylius\Component\Resource\State\ProviderInterface;
+
+final class BookItemProvider implements ProviderInterface


This file should be removed from git.

Prometee · 2023-09-01T10:50:06Z

src/Component/Symfony/Maker/Generator/ResourceClassGenerator.php

+        $shortName = $resourceClassDetails->getShortName();
+
+        if (\str_ends_with($shortName, 'Resource')) {
+            $shortName = \mb_substr($shortName, 0, -\strlen('Resource'));


Is it possible that $shortName === "" here ?

Prometee · 2023-09-01T10:51:25Z

src/Component/Symfony/Maker/MakeResource.php

+        $command
+            ->setDescription(self::getCommandDescription())
+            ->addArgument('name', InputArgument::OPTIONAL, 'Class name of the resource to create')
+            ->addOption('is-entity', null, InputOption::VALUE_NONE, 'Do you want to store resource data in the database (via Doctrine)?')


Suggested change

->addOption('is-entity', null, InputOption::VALUE_NONE, 'Do you want to store resource data in the database (via Doctrine)?')

->addOption('is-entity', null, InputOption::VALUE_NONE, 'Do you want to store resource data in the database (eg: via Doctrine)?')

Prometee · 2023-09-01T10:52:03Z

src/Component/Symfony/Maker/MakeResource.php

+    public function interact(InputInterface $input, ConsoleStyle $io, Command $command): void
+    {
+        $resourceIsEntity = $io->confirm(
+            'Do you want to store resource data in the database (via Doctrine)?',


Suggested change

'Do you want to store resource data in the database (via Doctrine)?',

'Do you want to store resource data in the database (eg: via Doctrine)?',

Prometee · 2023-09-01T11:04:43Z

src/Component/Symfony/Maker/MakeResource.php

+        /** @var string $name */
+        $name = $input->getArgument('name');
+
+        /** @var bool $resourceIsEntity */


Suggested change

/** @var bool $resourceIsEntity */

/** @var bool|null $resourceIsEntity */

Maybe it's better to set a default value for this option to avoid getting null at this point, WDYT ?

lchrusciel

awesome feature and huge effort. Thanks Loic! However, let's take small step and add just resource generation in the first PR. It would be easier to review. And we have inconsistencies among different makers. Let's build ground rules, decide what should be part of maker and then add more makers

lchrusciel · 2023-09-09T12:20:18Z

src/Bundle/DependencyInjection/Compiler/RegisterStubCommandsPass.php

+        if (!class_exists(MakerBundle::class)) {
+            return false;
+        }
+
+        /** @var array $bundles */
+        $bundles = $container->getParameter('kernel.bundles');
+
+        return in_array(MakerBundle::class, $bundles, true);


This logic skips case when the maker bundle is installed, but it turned off in the given env

lchrusciel · 2023-09-09T13:20:43Z

src/Component/Tests/Symfony/Maker/MakeResourceTest.php

+        return <<<EOF
+            <?php
+
+            namespace App\Tests\Tmp\Sylius\Resource;


Why do we have Sylius in the namespace for resources but not for processors? We have the command make:resource that have Sylius in a namespace, but then we have make:sylius-state-processor command, which generates processor without Sylius in namespace.

lchrusciel · 2023-09-09T13:21:39Z

src/Component/Tests/Symfony/Maker/MakeResourceTest.php

+            #[Index(
+                // grid: BookGrid::class,
+            )]


Suggested change

#[Index(

// grid: BookGrid::class,

)]

#[Index()]

My recommendation would be to remove all the comments from attributes. Or even all the routes in the first iteration

diimpp · 2023-09-12T12:33:38Z

src/Bundle/Resources/config/services/maker.xml

+
+ This file is part of the Sylius package.
+
+ (c) Paweł Jędrzejewski


--- (c) Paweł Jędrzejewski +++ (c) Sylius Sp. z o.o.

diimpp · 2023-09-12T13:26:22Z

src/Component/Symfony/Command/StubCommand.php

+            )
+        ;
+
+        return 0;


Exit codes consts are available since Symfony 5.4, so it could/should be used over here, but I'm unsure if it's within scope of this PR.

public const SUCCESS = 0; public const FAILURE = 1; public const INVALID = 2;

--- return 0; +++ return Command::SUCCESS;

loic425 requested a review from a team as a code owner April 14, 2023 15:10

loic425 marked this pull request as draft April 14, 2023 15:10

loic425 force-pushed the feature/make-resource branch 2 times, most recently from 40079e6 to 4317a1d Compare April 17, 2023 10:33

loic425 changed the title ~~Resource maker~~ [Maker] Resource maker Apr 17, 2023

loic425 force-pushed the feature/make-resource branch from 4317a1d to dd3135a Compare April 17, 2023 10:48

loic425 marked this pull request as ready for review April 17, 2023 13:46

loic425 force-pushed the feature/make-resource branch from 216e72a to 9528dfd Compare April 20, 2023 13:33

loic425 changed the title ~~[Maker] Resource maker~~ [Maker] Resource, State provider and state processor maker Apr 20, 2023

loic425 changed the title ~~[Maker] Resource, State provider and state processor maker~~ [Maker] Resource, State provider and state processor makers Apr 20, 2023

loic425 force-pushed the feature/make-resource branch 3 times, most recently from 1774077 to 5f623ea Compare June 20, 2023 13:11

loic425 force-pushed the poc-new-resource-metadata branch 2 times, most recently from b5a7698 to 5ad1d7d Compare June 20, 2023 13:59

Zales0123 added the Feature New feature proposals. label Jun 22, 2023

loic425 changed the base branch from poc-new-resource-metadata to 1.11 June 23, 2023 08:32

loic425 added 6 commits June 23, 2023 10:36

[Maker] Resource, State provider and state processor makers

c85aa8b

Move tmp directory to fix the Doctrine configuration

2c3d162

Fix testing makers

80550f8

Fix coding standard

0478696

Fix PHPUnit tests

638cc70

Fix coding standard

b414aba

loic425 force-pushed the feature/make-resource branch from 5f623ea to b414aba Compare June 23, 2023 08:38

loic425 commented Sep 1, 2023

View reviewed changes

Prometee reviewed Sep 1, 2023

View reviewed changes

lchrusciel reviewed Sep 9, 2023

View reviewed changes

diimpp reviewed Sep 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Maker] Resource, State provider and state processor makers #705

[Maker] Resource, State provider and state processor makers #705

loic425 commented Apr 14, 2023

Zales0123 commented Jun 22, 2023

loic425 commented Jun 23, 2023 •

edited

Loading

loic425 Sep 1, 2023

loic425 Sep 1, 2023

Prometee Sep 1, 2023

Prometee Sep 1, 2023

Prometee Sep 1, 2023

Prometee Sep 1, 2023 •

edited

Loading

lchrusciel left a comment

lchrusciel Sep 9, 2023

lchrusciel Sep 9, 2023

lchrusciel Sep 9, 2023

diimpp Sep 12, 2023

diimpp Sep 12, 2023

	->addOption('is-entity', null, InputOption::VALUE_NONE, 'Do you want to store resource data in the database (via Doctrine)?')
	->addOption('is-entity', null, InputOption::VALUE_NONE, 'Do you want to store resource data in the database (eg: via Doctrine)?')

	'Do you want to store resource data in the database (via Doctrine)?',
	'Do you want to store resource data in the database (eg: via Doctrine)?',

	/** @var bool $resourceIsEntity */
	/** @var bool\|null $resourceIsEntity */


		This file is part of the Sylius package.

		(c) Paweł Jędrzejewski

[Maker] Resource, State provider and state processor makers #705

Are you sure you want to change the base?

[Maker] Resource, State provider and state processor makers #705

Conversation

loic425 commented Apr 14, 2023

Zales0123 commented Jun 22, 2023

loic425 commented Jun 23, 2023 • edited Loading

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

Prometee Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

lchrusciel left a 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

Choose a reason for hiding this comment

loic425 commented Jun 23, 2023 •

edited

Loading

Prometee Sep 1, 2023 •

edited

Loading