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:entity improved: interactive field created & entity update support #104

Merged
merged 60 commits into from
Mar 17, 2018

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Jan 7, 2018

Hi guys!

The PR we (or at least me) have been waiting for :). This makes make:entity behave similar to doctrine:generate:entity from SensioGeneratorBundle: it interactively asks you for the field names and generates the getters/setters... but with some improved UX.

Features:

  • Ability to create new entities & fields
  • Ability to update existing entities 🎆
  • Support for adding relations (double 🎆)
  • A --regenerate flag to add getters/setters for existing properties (replaces doctrine:generate:entities)
  • Generates code that passes phpcs

Please help test! It's easy: composer require details to for testing

Blocker 😢

We're using the newest version of nikic/php-parser heavily... but it only has a beta release so far :(. We need to wait until this is stable. OR, we need to remove it as a dependency temporarily, then throw an error to tell people to install it manually (at the beta version). :/ nikic/PHP-Parser#474

Important notes:

  • I bumped the min php version to 7.1 (from 7.0). This was so that we could consistently generated code using nullable types.

  • Some of the code generated for relationships is opinionated. For example, I automatically synchronize (i.e. by calling the setter) the owning side if you set data on the inverse side of the relation. I also make all relation return types nullable (even if the relation is required in the db), to be friendly in case they're not set yet.

  • The setters are not fluent. We could make them fluent, or make that an option on the bundle (i.e. one in a config file, not asked interactively)

make:entity

If you want to see the available type list (relations are now in this list, but not shown):

screen shot 2018-01-08 at 3 09 32 pm

I would love feedback! I added many little "features" to make the user experience as excellent as possible. But, I need users to help validate that!

Cheers!

@@ -24,16 +22,76 @@ public static function validateClassName(string $className, string $errorMessage
if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/', $className)) {
$errorMessage = $errorMessage ?: sprintf('"%s" is not valid as a PHP class name (it must start with a letter or underscore, followed by any number of letters, numbers, or underscores)', $className);

throw new RuntimeCommandException($errorMessage);
throw new \InvalidArgumentException($errorMessage);
Copy link
Member

@yceruto yceruto Jan 7, 2018

Choose a reason for hiding this comment

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

These changes come back to the "ugly" exception message, what about to use Symfony\Component\Console\Exception\InvalidArgumentException instead? or create your own InvalidArgumentException that extends from it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. It doesn't make sense MOST of the time (i.e. it doesn't make sense if these are callbacks to a Question validation, where the exception is caught), but in case these methods are ever called in a different situation, better to throw the "prettier" exception.

Copy link
Member

@javiereguiluz javiereguiluz 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 really nice! I'm going to test it thoroughly in a real app and I'll report any issue. Thanks!

}
// check for valid PHP variable name
if (!is_null($name) && !Str::isValidPhpVariableName($name)) {
throw new \InvalidArgumentException(sprintf('"%s" is not a valid PHP variable name.', $name));
Copy link
Member

Choose a reason for hiding this comment

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

"%s" is not a valid PHP variable name -> "%s" is not a valid PHP property name ?

return $data;
}

private function printAvailableTypes(ConsoleStyle $io)
Copy link
Member

Choose a reason for hiding this comment

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

We could simplify this method as follows:

private function printAvailableTypes(ConsoleStyle $io)
{
    $io->write(' <info>Available types:</info> ');

    $types = array_keys(Type::getTypesMap());
    $io->write(wordwrap(implode(', ', $types), 50));
    $io->writeln('');
}

And if you want to keep the <comment> styling (adjust the word-wrap line-length accordingly):

private function printAvailableTypes(ConsoleStyle $io)
{
    $io->write(' <info>Available types:</info> ');

    $types = array_map(function ($v) { return '<comment>'.$v.'</comment>'; }, array_keys(Type::getTypesMap()));
    $io->write(wordwrap(implode(', ', $types), 80));
    $io->writeln('');
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I just rewrote this algorithm to be more complex, but make the output much fancier and easier for the user.

@weaverryan
Copy link
Member Author

Made a few tweaks and improvements! Ready for review again :)

}
// check reserved words
if ($this->isReservedKeyword($name)) {
throw new \InvalidArgumentException(sprintf('Name "%s" is a reserved word.', $name));
Copy link
Member

Choose a reason for hiding this comment

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

Could we allow it by quoting the column name? i.e.:

@Column(name="`number`", ...)

http://doctrine-orm.readthedocs.io/projects/doctrine-orm/en/latest/reference/basic-mapping.html#quoting-reserved-words
Doctrine will then quote this column name in all SQL statements according to the used database platform.

@weaverryan
Copy link
Member Author

I've also started support for relations/associations. You can see the beginning of how this will work (the command now supports relation types). But nothing generates yet :).

@codedmonkey
Copy link
Contributor

codedmonkey commented Jan 9, 2018

Very nice work. I like the idea of the ClassSourceManipulator, as it could be a basis for structured customization of the output, but it would need to be injectable for that.

I also like the idea of a relation helper. Here's my idea of a verbose guide that puts less emphasis on the words one and many and more on the collection aspect:

 Class name of the entity to create or update (e.g. GentleChef):
 > Article

 New field name (press <return> to stop adding fields):
 > sections

 Field type (enter ? to see all types) [string]:
 > relation

 What class should this entity be related to?:
 > Section

ManyToOne  The sections field is a reference to a single Section object.
           The Section object can have a field with a collection of Article objects.

OneToMany  The sections field is a collection of Section objects.
           The Section object has a field referencing a single Article object.

ManyToMany The sections field is a collection of Section objects.
           The Section object has a field with a collection of Article objects.
           A join table is created to store the relationships between the objects.

OneToOne   The sections field is a reference to a single Section object.
           The Section object can have a field referencing a single Article object.

Just an idea of course :) Keep up the good work.

@weaverryan weaverryan changed the title make:entity improved: interactive field created & entity update support [WIP] make:entity improved: interactive field created & entity update support Jan 9, 2018
@weaverryan
Copy link
Member Author

FYI = I've just set this back to WIP, and I'm going to finish the relation stuff in this PR.

@codedmonkey thanks for the quick review of things and UX suggestions! I actually already modified the text a bit further before reading this, but eventually, your version might be better. When I've fully finished the command (as I'm still messing around and doing all kinds of craziness), I would very much appreciate your review again! :)

@tvogt
Copy link

tvogt commented Jan 20, 2018

This is excellent. Will it work on existing Entities? Because most of the time, while prototyping, I will start with a simple entity, and then add fields and relations to it alter.

@weaverryan weaverryan force-pushed the make-entity-improved branch from 208a62c to 362d6d2 Compare January 26, 2018 02:35
@javiereguiluz javiereguiluz added the Feature New Feature label Feb 7, 2018
@TheKassaK
Copy link

Hello, i'm here because I'm looking for the old command : php bin/console doctrine:generate:entities for generate getters and setters... Any news ?

@biji
Copy link

biji commented Feb 7, 2018

Adding setter and getter to existing entity would be great

@weaverryan
Copy link
Member Author

I’ll be pushing the final version during the next day I hope. Generating getters/setters to an existing entity IS supported - I think you’ll like it ;)

@weaverryan weaverryan force-pushed the make-entity-improved branch 2 times, most recently from 0dfc1d5 to 7655423 Compare February 8, 2018 01:25
@weaverryan
Copy link
Member Author

Ok! Just added support for regenerating getters/setters (to replace doctrine:generate:entities) AND support for relationships! Wooo!

There is an external blocker for release (see above), but it's time to review and test. I'm quite confident in this feature, but (as you can see by the PR), it's quite complex.

.php_cs.dist Outdated
'protected_to_private' => false,
'semicolon_after_instruction' => false,
'header_comment' => [
'header' => <<<EOF
This file is part of the Symfony package.
Copy link
Member

Choose a reason for hiding this comment

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

wrong header. It should say of the Symfony MakerBundle package. This bundle is not in the core.

Copy link
Member

Choose a reason for hiding this comment

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

btw, this should probably be done in a separate PR, as it could then be merged.

.travis.yml Outdated
@@ -24,4 +23,4 @@ install:
- composer --prefer-dist install
- ./vendor/bin/simple-phpunit install

script: ./vendor/bin/simple-phpunit
script: ./vendor/bin/simple-phpunit --stop-on-failure --stop-on-error
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is meant to be reverted, right ?

/**
* Simpler version of DoctrineBundle's DisconnectedMetadataFactory, to
* avoid PSR-4 issues.
*
Copy link
Member

Choose a reason for hiding this comment

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

should be @internal IMO


private $orphanRemoval = false;

private $mapInverseRelation = true;
Copy link
Member

Choose a reason for hiding this comment

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

I would default to not creating the inverse relation. Unidirectional relations are much easier to handle in the long term in my experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of those tough spots. What are the actual downsides of mapping the inverse side? The generator actually generates setter/adder/remover methods on the inverse side that set the owning side, which makes the "I set the inverse side and nothing happened" problem lessened or completely eliminated.

Copy link

Choose a reason for hiding this comment

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

Inverse 1:1 assocations could be a performance nightmare. Also synchronization is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to add a question to ask the user if they want an inverse side, instead of always adding it. Then, it's their fault ;). Plus, we only synchronize the owning side (from the inverse side) to minimize performance. If you're not going to set the inverse side, then you won't call these methods and have the penalty anyways. But if you do set the inverse side, then it's really nice to synchronize and I think the penalty is worth it.

// both overridden below for OneToMany
$newFieldName = $newField->getOwningProperty();
$otherManipulatorFilename = $this->getPathOfClass($newField->getInverseClass());
$otherManipulator = $this->createClassManipulator($otherManipulatorFilename, $io, $overwrite);
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 create this one only when needed (i.e. when there is a birectional relation) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a bit wasteful, but it just gets ugly the other way - we use it a few places below, except for OneToMany, where we override it. I tried wrapping this in an if statement and it only seemed to make things look worse.

src/Str.php Outdated
@@ -87,6 +99,11 @@ public static function asRouteName(string $value): string
return self::asTwigVariable($value);
}

public static function snakeCase(string $value): string
Copy link
Member

Choose a reason for hiding this comment

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

should be asSnakeCase for consistency

private function buildAnnotationLine(string $annotationClass, array $options)
{
$formattedOptions = array_map(function ($option, $value) {
$quoteValue = !(is_bool($value) || is_int($value) || is_array($value));
Copy link
Member

Choose a reason for hiding this comment

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

what about null ?

}

$value = sprintf('{%s}', implode(', ', array_map(function ($val) {
return sprintf('"%s"', $val);
Copy link
Member

Choose a reason for hiding this comment

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

this only supports string values

$typeHint,
// make the type-hint nullable always for ManyToOne to allow the owning
// side to be set to null, which is needed for orphanRemoval
// e.g. $userAvatarPhoto->setUser(null);
Copy link
Member

Choose a reason for hiding this comment

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

that's wrong. OrphanRemoval does not require setting it to null. It only requires removing it from the inverse collection.

Copy link
Member

Choose a reason for hiding this comment

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

or, the case of ManyToOne vs OnetoOne are swapped

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, this is confusing. The issue is that the generated remove* method on the inverse side (and really, the mutators for all the inverse sides) synchronizes the owning side - e.g. it looks like this:

// User.php
public function removeAvatarPhoto(UserAvatarPhoto $photo)
{
    $this->avatarPhotos->removeElement($photo);
    // This is the key: we sync the owning side, which is why it needs to be nullable
    $photo->setUser(null);
}

When orphanRemoval is set, yea, we don't need to sync the owning side, as the orphanRemoval will cause the deletion. But since we are sync'ing it (and I think that's generally a good idea - and I don't want to NOT sync it just for the case of orphanRemoval=true - want to not create so many variations), we need to allow the setter to be null.

These are the tough balances

Copy link
Member

Choose a reason for hiding this comment

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

but this syncing forces you to make the setter accept null on the owning side, even if null would be an invalid value because of the field being non-nullable, allowing invalid usage of the class.

case 'array':
case 'simple_array':
case 'json_array':
case 'json':
Copy link
Member

Choose a reason for hiding this comment

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

not good. You can store other things than arrays in a json field (even though it is the common need)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, check out the JsonType class - it uses $val = json_decode($value, true); in convertToPHPValue. So it looks like it does always return an array.

However, that method could also return null. So probably having no return type is safer.

Copy link
Member

Choose a reason for hiding this comment

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

nope, it can also return any scalar or null. The true argument just means it will never return you a stdClass


public function setUser(?User $user)
{
$this->user = $user;
Copy link

Choose a reason for hiding this comment

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

What if I update this directly? Then User#avatars will not be updated

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean: if you call $avatar->setUser($user), the problem is that the avatar will not all be added to User#avatars? i.e. the problem is that the relationship is not synced?

If so, it’s by design. If you set the inverse side of a relation, the generated code does sync the opening side. But not the other way around. The reason is that (a) it doesn’t need to be set for persistence and (b) more importantly, it could have major performance consequences. For example, if there are 1000 avatar, by setting User#avatars, Doctrine would first query for all 1000 and then set it. We gotta avoid that :)

Copy link

Choose a reason for hiding this comment

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

Here's an example:

  1. Create a new article, setCategory($category).
  2. Something later (eg. listener) grabs $article->getCategory() and passes it somewhere else
  3. It ends up in code that does foreach ($category->getArticles() as $article)
  4. Weird results

I understand what you are saying but relying on chance that the collection will not be used later during request.... not really sound strategy.

Doctrine would first query for all 1000 and then set it

Not is it's set to extra lazy: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/tutorials/extra-lazy-associations.html

Copy link
Member Author

Choose a reason for hiding this comment

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

So... this is tricky. If we generate code that has fetch="EXTRA_LAZY", then setting the inverse side, only costs one "fast" to check of the item already is related. If we do not set "EXTRA_LAZY", then it is far too costly.

This leaves us with 2 choices:

  1. Generate all relationships as EXTRA_LAZY, sync the inverse side (just for edge-case convenience) and suffer a small perf impact

  2. Do no generate relationships as EXTRA_LAZY, do not sync the inverse side (edge case problems could arise) and suffer no per impact.

I'm really not convinced either way. @stof @Majkl578 you usually have words of warning - what do you have here? Btw, we now ask the user IF they want to generate the inverse side. If they choose no, we don't have to worry about any of this.

I lean slightly towards option (1). EXTRA_LAZY seems better in 99% of the cases. And I don't like the small perf impact, but it really is small, and avoids WTF situations. I can't think of a situation where this causes a BIG perf impact.

Copy link
Member

Choose a reason for hiding this comment

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

EXTRA_LAZY seems better in 99% of the cases.

That's not actually true. If your business need means that the collection will never become huge, performing DB queries each time you check contains or count or similar APIs on the collection is actually much worse for performance than loading the collection on first usage (trading a bit of memory to win on time). I actually reverted a bunch of my extra lazy collections to be only lazy ones (i.e. the default) to solve performance issues.

For me, the right fix is to stop generating bidirectional relations by default. All this comes from the fact that the code expects a fully synchronized bi-directional relation. If you make the relation unidirectional, you make all these issues disappear (if you try to load the list of all articles before saving the change the DB, you might have a harder time writing your listener indeed as there is no place having the source of truth for the updated full list yet, but then it might push you to reconsider this need instead of killing performance and making the entity code very complex to have this list available).

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought more about this too and had the same thought as you Stof - that EXTRA_LAZY could cause tons of count queries in certain situations.

If you make the relation unidirectional, you make all these issues disappear

It's true. But you've also killed a lot of usability for many users. To strike a balance, the command now DOES ask if you want the inverse side to be mapped. So, it's up to you

@weaverryan weaverryan changed the title [WIP] make:entity improved: interactive field created & entity update support make:entity improved: interactive field created & entity update support Feb 10, 2018
@Kudze
Copy link

Kudze commented Feb 12, 2018

I've tested it. Looks really nice, however I have some suggestions for make:entity.

When generating a string generate:doctrine:entity used to ask for length, and this one doesn't. In my opinion it should ask for it.

Also an option for setters that would return the object would be nice. (If you can't understand what i mean by setters that would return the object, heres an example)

class Entity {

    private $value;

    //...

    public funcition setValue(string $value) : Entity
    {
        $this->value = $value;

        return $this;
    }

    //...

}

This would allow to set a lot of variables at once like this:

$entity->setValue1("val1")->setValue2(420);

@nckenn
Copy link

nckenn commented Mar 1, 2018

nikic/PHP-Parser#474 has now a stable version, hope you can make release version of this PR :)

@weaverryan weaverryan force-pushed the make-entity-improved branch from 1f3a83c to b0fed52 Compare March 1, 2018 21:06
@weaverryan
Copy link
Member Author

Ok! I've just updated to use nikic/PHP-Parser 4.0 stable!

There are no known, pending issues with this PR. So, now would be the time for anyone to do any last review or testing.

The tests on appveyor ARE randomly failing - different tests each time :/. I've started debugging them, but the circumstances are very odd. As long as no appveyor tests consistently fail, I'm not going to let that block this PR (i.e. we'll debug the phantom failures later).

Cheers!

@nicodemuz
Copy link

@weaverryan regarding the randomly failing tests, maybe you can let your CI script echo out the User entity that seems to have the missing methods.

Maybe it's a file permission issue or the disk is full?

@weaverryan
Copy link
Member Author

Thanks for checking the tests :). I actually did that earlier from inside the test class that reports them missing. Ready for your mind to be blown? The methods were there! It’s as if the test class in the tmp project already have an old version of the class in memory, but I don’t know how that’s possible, or what on windows would cause it. I’m open to ideas :)

@Gwynei
Copy link

Gwynei commented Mar 6, 2018

This is the most wanted feature for me in Symfony 4.

I have tested it, and I have two suggestions:

  • Is it possible to regenerate ALL entities at once? Not that all entities will be changed, but thing is that it speeds up developing. For example, I make changes in six entities (deleted ManyToOne, changed data type, added field, etc), now I have to run the --regenerate command six times and I have to find out the exact names of those six entities. In Symfony 3.4 I could run doctrine:generate:entities App and it would check/update every entity.

  • Minor one: When generating an entity and you use the maker to generate fields, the // your fields here comment is at the bottom, after the $id field and the GetId() getter. Previously it was between those two (I usually have all my fields first, and then the getters/setters.

Thanks a bunch!

[EDIT]
Found a bug (I think) fields with nullable=true give this error while regenerating:

2018-03-06T09:29:26+01:00 [error] Error thrown while running command "make:entity --regenerate". ` Message: "Notice: Undefined index: nullable"

In EntityRegenerator.php line 127:

Notice: Undefined index: nullable

@weaverryan
Copy link
Member Author

Thanks for testing @Gwynei :). About your questions/comments:

Is it possible to regenerate ALL entities at once? Not that all entities will be changed, but thing is that it speeds up developing. For example, I make changes in six entities (deleted ManyToOne, changed data type, added field, etc), now I have to run the --regenerate command six times and I have to find out the exact names of those six entities. In Symfony 3.4 I could run doctrine:generate:entities App and it would check/update every entity.

Yes, you should be able to just do bin/console make:entity --regenerate and use the default App value (i.e. the namespace, not the class name) for the one question. You can regenerate a single class, or all classes matching a namespace. Let me know if you're seeing otherwise.

Minor one: When generating an entity and you use the maker to generate fields, the // your fields here comment is at the bottom, after the $id field and the GetId() getter. Previously it was between those two (I usually have all my fields first, and then the getters/setters.

Good catch! I'm going to remove the comment entirely.

Found a bug (I think) fields with nullable=true give this error while regenerating:

2018-03-06T09:29:26+01:00 [error] Error thrown while running command "make:entity --regenerate". ` Message: "Notice: Undefined index: nullable"
In EntityRegenerator.php line 127:
Notice: Undefined index: nullable

Another good catch - I should have it fixed now :). But let me know if you see anything else like this.

@Gwynei
Copy link

Gwynei commented Mar 12, 2018

Thanks for the fixes @weaverryan - works fine now!

If I may do one more request? Can you make it so that I can give a parameter to the entity/entities to regenerate? Such as this:

bin/console make:entity --regenerate App
or
bin/console make:entity --regenerate App\Entity\User

Not mandatory but just a convenience. :)

if (is_array($value)) {
if (!isset($value[0])) {
// associative array: we'll add this if/when we need it
throw new \Exception('Not currently supported');

Choose a reason for hiding this comment

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

$value = ["default" => "now()"] fires the exception

Copy link
Member Author

Choose a reason for hiding this comment

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

It is true that you could use the ClassSourceManipulator directly (specifically addEntityField()) and pass in some annotations where the value is an associative array. But, this situation is not actually possible in the bundle (at least at this time) - either addEntityField() is called by MakeEntity, and it passes in only scalar values, or it's called by EntityRegenerator, but in that case, the annotations aren't actually printed on the field.

So, it's just not a possible use-case right now :). If someone wants to PR this to fill it in, that's cool. I don't want to fill it in quickly and realize it's incorrect if we do in fact later use it.

* master: (47 commits)
  Updating CHANGELOG
  some improvements
  fix form variables
  added validation on arguments
  fix phpcs
  fix variables to follow official recommendations
  some make:form improvements + experiments
  final fix for skeleton templates
  making test name less specific so we can run only it
  Update index.rst
  phpcs fix
  some skeleton improvements
  Fixed CS of generated entity
  fix cs and improvements according to review request
  fix make:form
  completed improvement of make:form command
  added tests for crud testig
  fix tests on appveyor
  php cs fix
  update all to lastest bundle code
  ...
@weaverryan
Copy link
Member Author

@Gwynei good catch - I've fixed the command so you can just run bin/console make:entity --regenerate App and not get the interactive question - that was indeed the intention :)

@nicodemuz
Copy link

nicodemuz commented Mar 16, 2018

Can we add an option to skip generation of annotations? I'm using YML.

@nicodemuz
Copy link

Adding a comment on an field does not work:

App\Entity\Testing:
    type: entity
    table: testing
    repositoryClass: App\Repository\TestingRepository
    id:
        id:
            type: integer
            id: true
            generator:
                strategy: AUTO
    fields:
        created:
            type: datetime
            options:
                comment: Date of creation.

It gives me the following error:

In ClassSourceManipulator.php line 211:

  Not currently supported

@weaverryan
Copy link
Member Author

@MrNicodemuz

Can we add an option to skip generation of annotations? I'm using YML.

I don't understand what you mean. If you use YAML, you cannot use this command to add new fields: you can only use the --regenerate option. And when you use the --regenerate option, it only generates the properties, getters, setters, etc - it does not generate annotations above the properties. If you see something different, please let me know :).

It gives me the following error:

This is legit - I just finished telling someone this situation wasn't possible. I was wrong :). I'll fix it.

@weaverryan weaverryan merged commit 16596b1 into symfony:master Mar 17, 2018
weaverryan added a commit that referenced this pull request Mar 17, 2018
… update support (weaverryan)

This PR was squashed before being merged into the 1.0-dev branch (closes #104).

Discussion
----------

make:entity improved: interactive field created & entity update support

Hi guys!

The PR we (or at least me) have been waiting for :). This makes `make:entity` behave similar to `doctrine:generate:entity` from SensioGeneratorBundle: it interactively asks you for the field names and generates the getters/setters... but with some improved UX.

## Features:

* Ability to create new entities & fields
* Ability to update *existing* entities 🎆
* Support for adding *relations* (double 🎆)
* A `--regenerate` flag to add getters/setters for existing properties (replaces `doctrine:generate:entities`)
* Generates code that passes phpcs

Please help test! It's easy: [composer require details to for testing](https://gist.github.com/weaverryan/a20fc281ccd1faea2ea04a5da7fdaa55)

## Blocker 😢

We're using the newest version of `nikic/php-parser` heavily... but it only has a *beta* release so far :(. We need to wait until this is stable. OR, we need to remove it as a dependency temporarily, then throw an error to tell people to install it manually (at the beta version). :/ nikic/PHP-Parser#474

## Important notes:

* I bumped the min php version to 7.1 (from 7.0). This was so that we could consistently generated code using nullable types.

* Some of the code generated for relationships is opinionated. For example, I automatically synchronize (i.e. by calling the setter) the *owning* side if you set data on the inverse side of the relation. I also make all relation return types nullable (even if the relation is required in the db), to be friendly in case they're not set yet.

* The setters are not fluent. We *could* make them fluent, or make that an option on the bundle (i.e. one in a config file, not asked interactively)

![make:entity](https://i.imgur.com/Sflti0y.gif)

If you want to see the available type list (relations are now in this list, but not shown):

<img width="535" alt="screen shot 2018-01-08 at 3 09 32 pm" src="https://user-images.githubusercontent.com/121003/34690183-21f7498e-f486-11e7-9fcf-a5fe56d2a321.png">

I would love feedback! I added many little "features" to make the user experience as excellent as possible. But, I need users to help validate that!

Cheers!

Commits
-------

16596b1 updating tests for removed comment
01f6a3c Fixing a bug where a repository would not be generated under certain conditions
2f0e0f3 adding support for associative arrays in annotations
7d3042c Fixing bug where the name was always interactive
2a72ee3 Fixing cs
3b65fb6 Merge branch 'master' into make-entity-improved
e54988c Fixing issue where sometimes nullable is not defined on a join column
259141d removing comment - it does not add much value & is hard to keep in the right place
b0fed52 updating to stable nickic/php-parser 4.0 release!
7bcc966 Disabling replaceNodes to avoid possible AST accidental changes
613abef Adding an option (true by default) for fluent setter methods
8634a91 cs
9261cd1 Adding the "length" question for the string type
9c5a786 phpcs
328e486 fixing windows bug
92aeff5 playing with appveyor
b3647d7 attempting to split tests to combat windows testing issue
f022f5f Fixed window path bug while looking for the vendor/ dir
89d5708 trying to fix windows tests with long paths that cause errors :/
fd088ac temporarily debugging Windows errors
35ef3ae removing duplicate sqlite extension for appveyor
8c37893 Making appveyor use sqlite for simplicity
abbe8d9 Fixing windows bug where C:/ slash may already be normalized
20df636 Fixing a few Windows bugs
f9fa0db adding missing success message
38e6a0e Fixing missing return on --regenerate
1650301 Fixing missing method
fb74302 Fixing typo for class name
9c40654 added missing service
866e534 Extracting logic into AutoloaderUtil
05480da Fixing bug where new classes were never actually written in regenerator
e535f0d Adding an error if you try to generate a class that does not use annotation metadata
656c199 Fixing 2 merge bugs: entities could not be updated & mix up of entity vs repo class name
8d400e7 Fixing bug with getting property of classes just created
e8e5689 fix arg name
b8856c0 Code changes to sync with upstream updates
4afbc0e removing dup functions after merge
a34a1a2 Fixing "composer require orm orm" bug
5bc34bd removing unused use
d34ff45 also run phpcs on updated files
f370b52 fixing tests
5395788 Not setting the owning side (from inverse) on OneToOne
922563a clarifying note
ab03675 fixing missing return type
cf9e08a making decimal/scale more clear
0f2d52d Removing unnecessary service locator: just let the registry be null
1f57317 minor tweaks
d65d772 Fixing some tests
a822400 adding missing headers
dc7973d Fixing tests + one missing return
c3ccf5f Ask the user if they want the inverse side of a relation
2fd2e0c Making the user message a little nicer
663afee test needs database
c79b73c Fixes thanks to Stof!
6c914fa Removing debugging code, committing missing file to fix tests
4bde1dc With --overwrite, put updated methods in same location as original
ed15a2b temp debugging code for failing test on CI
dbe11a5 hack to fix tests temporarily
cd4f517 Fixing possible non-existent directory in tests
e52e33e Adding an enhanced make:entity command
@mnavarrocarter
Copy link

This is amazing. Thanks @weaverryan!

}, $value, array_keys($value))));

// associative array: we'll add this if/when we need it
throw new \Exception('Not currently supported');
Copy link
Member

Choose a reason for hiding this comment

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

this is dead code

@deivid11
Copy link

Thanks! So wanted feature! @weaverryan 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.