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

[make:crud|voter] generate classes with final keyword #1539

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jrushlow
Copy link
Collaborator

@jrushlow jrushlow commented May 6, 2024

  • introduces 2 new configuration values:

    • generate_final_classes: Defaults to true - When true, all non-entity classes generated by MakerBundle will be final.
    • generate_final_entities: Defaults to false - When true, all entity classes generated by MakerBundle will be final.
  • introduces an internal ClassData object that represents all of the meta data needed to generate a class from a template. It combines parts of ClassNameDetails & UseStatementGenerator, while adding logic to generate a class declaration conditionally including the finalkeyword &extends` class.

  • ClassData is configured by TemplateComponentGenerator to handle root_namespace, generate_final_classes, & generate_final_entities if passed as a template class_data variable to Generator::createClass(). The idea being we don't have to modify the constructor for the "non-internal" Generator::class

  • reduces the complexity of our .tpl.php templates. The ClassData object is accessible from within the templates (Crud & Voter for now). $class_data->getClassDeclaration() generates final class SomeClass extends XyzClass

  • To keep the PR's "reviewable" this PR only adds the new functionality for make:crud & make:voter. To be implemented in additional makers in subsequent PR's.

  • fix test fixtures to use final

  • update this description to include all of the proposed changes

fixes #1536


I think instead of one massive PR, we will merge this PR first to include make:voter & make:crud -> create subsequent PR's for makers that touch entities (1 pr) and the rest of the makers (1 pr). I say this because this PR is actually adding a handful of new features (mostly internal) in order to generate final classes.

@jrushlow jrushlow added Feature New Feature Status: Needs Review Needs to be reviewed labels May 6, 2024
@seb-jean
Copy link
Contributor

seb-jean commented May 6, 2024

Thanks Jesse

@jrushlow
Copy link
Collaborator Author

jrushlow commented May 6, 2024

Hmm. So I don't think we can final classes by default without causing confusion amongst the consumers of MakerBundle. I'm not 100% on my thought process here, but doctrine orm is using lazy ghosts by default - atleast that's what we're doing in the DoctrineBundleRecipe -> https://github.com/symfony/recipes/blob/8cba886fb716a49e654845980cba33e211288a9b/doctrine/doctrine-bundle/2.9/config/packages/doctrine.yaml#L11

Like the failed tests for this PR, wouldn't we trigger logic exceptions everywhere in userland code similar to:

Symfony\Component\VarExporter\Exception\LogicException: Cannot generate lazy ghost: class "Symfony\Bundle\MakerBundle\Tests\tmp\current_project\src\Entity\User" is final.

if all the generated entities were final, we'd end up forcing the user to either a) remove the final keyword everytime they make:user|entity|security etc... || b) set orm.enable_lazy_ghost_objects: false. Both of those options are not viable.

I think the only way we can implement the final keyword for generated entities would be on a opt-in basis with a --is-final flag. We should be able to implement the final keyword for non-entity classes by default.

If I'm missing something obvious - please let me know.. Thoughts @symfony/mergers ?

@jrushlow jrushlow changed the title [make:*] generate all classes with final keyword [make:crud|voter] generate classes with final keyword May 19, 2024
@jrushlow jrushlow added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use final classes
2 participants