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

Deprecating doctrine:generate:entities and pointing to MakerBundle #790

Merged
merged 2 commits into from
Mar 25, 2018

Conversation

weaverryan
Copy link
Contributor

Since the Doctrine devs aren't interested in maintaining the generation commands and since we implemented them in MakerBundle, I hope this will be a nice win for everyone: let's point them elsewhere and avoid issues being created related to Symfony 4 support :).

@@ -72,6 +72,9 @@ class is supposed to extend which. You have to adjust the entity
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
trigger_error('The doctrine:generate:entity command has been deprecated.', E_USER_DEPRECATED);
$output->writeln(' <comment>NOTE:</comment> The <info>doctrine:generate:entities</info> command has been deprecated. Use <info>make:entity --regenerate</info> from MakerBundle instead.');
Copy link
Member

Choose a reason for hiding this comment

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

I'd likely suggest pointing out the bad practice and point it at the docs instead. We (doctrine project) do not endorse MakerBundle in any way, as it just perpetuates an already too widespread misuse of mappings.

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest changing the language to something like this:

$output->writeln(' <comment>NOTE:</comment> The <info>doctrine:generate:entities</info> command has been deprecated. To read more about the differences between anemic and rich models go here <link to our docs>. If you wish to generate your entities, use <info>make:entity --regenerate</info> from MakerBundle instead.');

Copy link
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

Agree with @Ocramius, suggesting bad practice is bad.

Copy link
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

oops, wrong button

@Nyholm
Copy link
Contributor

Nyholm commented Mar 23, 2018

I like the idea of deprecating this command. How about writing a message like:

NOTE: The doctrine:generate:entities command has been deprecated. Read more at https://symfony.com/doc/current/bundles/DoctrineBundle/index.html

(obviously we need to update that page to say WHY this is a bad practice)

@weaverryan
Copy link
Contributor Author

I like @Nyholm's idea. DoctrineBundle is the intersection of Symfony & Doctrine. This would allow us to give your recommendation on the matter, but also point people to the Symfony resource that is available, should they choose it.

@Majkl578
Copy link
Contributor

There is currently no recommendation regarding this and Symfony explicitly rejected to do so. So at current state it's a no from me.
In meanwhile, Doctrine documentation has been updated.

@Nyholm
Copy link
Contributor

Nyholm commented Mar 23, 2018

Thank you for your input. I have not seen these PRs before.

So if I understand you correctly you are fine with this PR if there is a section in the docs that explains why this command was deprecated. The symfony docs will also say something like: "If you still really want this generate feature, then look at the MakerBundle".

@Ocramius
Copy link
Member

Read more at https://symfony.com/doc/current/bundles/DoctrineBundle/index.html

Great idea if the docs are coming from this repo, but since we disagree on practices (as @Majkl578 pointed out): no.

Link http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/tutorials/getting-started.html#adding-behavior-to-entities instead.

@weaverryan
Copy link
Contributor Author

Hi guys!

I've just updated the message to talk about anemic models and link to the new section on the Doctrine documentation. THEN, at the end, I mention the new command if the user still wants it. It looks like this:

screen shot 2018-03-24 at 7 55 22 am

If you guys want to educate on anemic versus rich models, this is much better than just having a command that doesn't work :).

@kimhemsoe
Copy link
Member

+1 from here.

Will merge if no objections.

Is there any symfony documentation about using dto's + doctrine with forms vs letting using form + doctrine? If not i believe it could be a nice addition.

@kimhemsoe kimhemsoe self-assigned this Mar 24, 2018
@weaverryan
Copy link
Contributor Author

@kimhemsoe

Is there any symfony documentation about using dto's + doctrine with forms vs letting using form + doctrine? If not i believe it could be a nice addition.

Not really. And we talked internally, and I think this is something we need to improve - both showing forms with model objects (or at least mentioning that more specifically) and also showing data mappers, which already has a PR (symfony/symfony-docs#6900) we just need to "un-stuck" it :).

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

This is on an acceptable middle-ground now 👍

@kimhemsoe kimhemsoe added this to the 1.8.2 milestone Mar 25, 2018
@kimhemsoe kimhemsoe merged commit 7f2a013 into doctrine:master Mar 25, 2018
@kimhemsoe
Copy link
Member

@weaverryan Thanks

@weaverryan weaverryan deleted the pointing_to_maker branch March 25, 2018 18:46
@jwage
Copy link
Member

jwage commented Mar 25, 2018

Thanks @weaverryan and @Ocramius!

@kimhemsoe kimhemsoe modified the milestones: 1.8.2, 1.9.0 Apr 18, 2018
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.

8 participants