search

symfony / maker-bundle

Symfony Maker Bundle
https://symfony.com/
MIT License
3.36k stars 406 forks source link

[RFC][make:entity] Add an option to generate fluent or not fluent setters #297

Open maks-rafalko opened 6 years ago

maks-rafalko commented 6 years ago

Hi everyone!

The only one mention about fluent setters I found is here: https://github.com/symfony/maker-bundle/pull/104, but nothing has been implemented as far as I see from the source code.

There is a flag that checks whether to generate fluent setters or not, here

https://github.com/symfony/maker-bundle/blob/705d3a2194d2d151299d4c3d61dc03dea7494357/src/Util/ClassSourceManipulator.php#L947-L951

but it's impossible to override the default value (which is true - setters are fluent), because manipulator class is instantiated always with true:

https://github.com/symfony/maker-bundle/blob/705d3a2194d2d151299d4c3d61dc03dea7494357/src/Maker/MakeEntity.php#L769

Note the last parameter

https://github.com/symfony/maker-bundle/blob/705d3a2194d2d151299d4c3d61dc03dea7494357/src/Util/ClassSourceManipulator.php#L57

Proposal

What do you think about adding an option that will allow overriding this flag? For example:

bin/console make:entity --fluent-setters=false

or something like that.

If this would be useful from your point of view, I would be able to send a PR.

javiereguiluz commented 6 years ago

@borNfreee thanks for your proposal. At first sight I'd say we shouldn't make this configurable ... because the current behaviour is configurable (more precisely, it's flexible).

If you generate fluent setters, you can decide if you prefer to use them fluently or not. Both ways work. But if you don't make them fluent or make it configurable ... user must check which is the format to use, etc.

maks-rafalko commented 6 years ago

But if you don't make them fluent or make it configurable ... user must check which is the format to use, etc.

Could you please explain what do you mean here?

Some background of why I think there should be an option:

  1. We are using api-platform and Sonata Admin, and to take everything from this 2 libraries, unfortunately, we have to use getters/setters.

  2. When we generate entities, we have to go through each method and replace return type, and remove return $this line

  3. Developer can miss one of the methods, ending up with 2 types of setters - one is fluent, another is not (just a human mistake, that's easy)

  4. When you use a fluent setter (alone, not as a chain), for example, $user->setName('John');, my IDE with PhpInspection EA Extended plugin complains that the returned value is not used and is being ignored, underlining it with a red line which is quite annoying.

  5. https://ocramius.github.io/blog/fluent-interfaces-are-evil/

Nyholm commented 5 years ago

If this should be implemented, then we maybe should support it to be configurable with the bundle's configuration.

weaverryan commented 5 years ago

Yea, I'd be fine with this - as configuration in the bundle itself (not as an interactive question)... which I think would work better for your use-case anyways (one consistent way of generating entities through a project, every time).

Someone just needs to add the config and make the generate respect it - it's a "fairly" easy change :).

Cheers!

Nyholm commented 5 years ago

If someone tries to implement this: remember that some generated code rely on these fluid interfaces.