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

Add maker command to generate data providers/persisters #3850

Merged
merged 9 commits into from
Jun 18, 2021

Conversation

antograssiot
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tickets fixes #3849
License MIT
Doc PR no yet

I had this ready since a year probably for discussion

@soyuka
Copy link
Member

soyuka commented Nov 26, 2020

This is a really good start thanks! Maybe some improvements for later:

  • generate the support methods by asking what entity this data provider / persister targets
  • DataProvider: choose if you want to do collections, items or both (not sure if this is possible)

@antograssiot
Copy link
Contributor Author

I'll look into this soon

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

This looks great!

src/Bridge/Symfony/Maker/MakeDataPersister.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Maker/MakeDataPersister.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Maker/MakeDataPersister.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Maker/MakeDataProvider.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Maker/MakeDataProvider.php Outdated Show resolved Hide resolved
@weaverryan
Copy link
Contributor

I love this idea!

What about asking a question related to decoration? I commonly want a data persister or data provider simply so that I can decorate the core persister/provider to add some functionality - this is mostly useful with entities. The question could be:

Will this data persister be used for an entity class?

(if yes):

Choose which entity:

A) Foo
B) Bar
C) Other entities...

And then... I think we wouldn't even need to ask them if they want to "decorate"/call the core persister... I would think that if you have an entity then you always would.

Or, another option would be to ask:

Choose which API Resource class this data persister will be used for?

A) Foo
B) Bar
C) ... list all other API Resource classes
D) None/other (in case the user is doing something super custom)

Then, if the class is an entity, we decorate the core entity persister.

It definitely adds significant complexity... but it would also be very nice, so I wanted to at least mention the idea :).

Thanks!

@antograssiot antograssiot force-pushed the maker branch 3 times, most recently from 9261275 to b7b972b Compare November 27, 2020 20:33
@antograssiot
Copy link
Contributor Author

I've improved the initial proposal by including @soyuka and @dunglas proposals

You can now choose in data providers it you only want to generate item or collection data providers
I'va also added a new question regarding the target resource class with auto-completion

@antograssiot antograssiot force-pushed the maker branch 4 times, most recently from 2423cb8 to 32e1dbb Compare November 28, 2020 08:54
@antograssiot antograssiot marked this pull request as ready for review November 28, 2020 11:21
@antograssiot
Copy link
Contributor Author

updated as requested, I've also improve code coverage and rebased @dunglas
I like @weaverryan suggestion on the decoration but it might fit in a future PR, I'm not sure when I'll have time to work on this, probably not before Christmas holidays

@antograssiot antograssiot requested a review from dunglas December 5, 2020 10:02
Base automatically changed from master to main January 23, 2021 21:59
@soyuka soyuka added this to the 2.7 milestone Feb 6, 2021
Comment on lines 615 to 617
->arrayNode('maker')
->{class_exists(MakerBundle::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->end()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->arrayNode('maker')
->{class_exists(MakerBundle::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->end()
->arrayNode('maker')
->{class_exists(MakerBundle::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->end()

{
return 'make:data-persister';
}

Copy link
Member

Choose a reason for hiding this comment

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

You should add the getCommandDescription static method instead of a call to setDescription (seems new to make it lazy).

->addOption('collection-only', null, InputOption::VALUE_NONE, 'Generate only a collection data provider')
->setHelp(file_get_contents(__DIR__.'/Resources/help/MakeDataProvider.txt'));

$inputConfig->setArgumentAsNonInteractive('name');
Copy link
Member

Choose a reason for hiding this comment

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

Why not using the automatic prompt for the name?

@@ -0,0 +1,5 @@
The <info>%command.name%</info> command generates a new data persister class.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The <info>%command.name%</info> command generates a new data persister class.
The <info>%command.name%</info> command generates an API Platform data persister class.

@@ -0,0 +1,5 @@
The <info>%command.name%</info> command generates a new data provider class.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The <info>%command.name%</info> command generates a new data provider class.
The <info>%command.name%</info> command generates an API Platform data provider class.

@@ -0,0 +1,47 @@
<?= "<?php\n" ?>

Copy link
Member

Choose a reason for hiding this comment

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

No strict type?

use <?= $resource_full_class_name ?>;
<?php endif; ?>

final class <?= $class_name ?> implements ContextAwareDataPersisterInterface
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we add the ResumableDataPersisterInterface too by default?

@@ -1265,6 +1265,9 @@ private function getBaseContainerBuilderProphecy(array $doctrineIntegrationsToLo
'api_platform.json_schema.schema_factory',
'api_platform.listener.view.validate',
'api_platform.listener.view.validate_query_parameters',
'api_platform.maker.command.data_persister',
'api_platform.maker.command.data_provider',
'api_platform.mercure.listener.response.add_link_header',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'api_platform.mercure.listener.response.add_link_header',

@alanpoulain
Copy link
Member

I love it! 😍
I think we can merge it as is and improve it later (for instance with @weaverryan's comment) later.
WDYT @antograssiot @api-platform/core-team?

@soyuka
Copy link
Member

soyuka commented Feb 23, 2021

@alanpoulain feel free to continue working on this branch as @antograssiot doesn't have much time to continue!

@alanpoulain alanpoulain merged commit e19c9be into api-platform:main Jun 18, 2021
@alanpoulain
Copy link
Member

Thank you very much @antograssiot!

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

Successfully merging this pull request may close these issues.

Create maker commands to generate data providers and data persisters
5 participants