symfony / maker-bundle

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

[make:entity] Maker modifies vendor files if FQCN covers up local alias #1566

Open pkly opened 5 months ago

pkly commented 5 months ago

Using 1.59.1 the following interaction creates a funny edit in the vendor directory.

I earlier created entities under our regular doctrine mapping, with MangoPay as the preceding namespace. I then named the entity Wallet, so a class under App/Common/Entity/MangoPay/Wallet exists.

The doctrine mapping for this section is as following:

        mappings:
          App:
            is_bundle: false
            type: attribute
            dir: '%kernel.project_dir%/src/Common/Entity'
            prefix: 'App\Common\Entity'
            alias: App

The following interaction then occurs.

# bin/console make:entity

 Class name of the entity to create or update (e.g. VictoriousElephant):
 > MangoPay\Wallet

 Your entity already exists! So let's add some new fields!

 New property name (press <return> to stop adding fields):
 > stuff

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

 Field length [255]:
 > 

 Can this field be null in the database (nullable) (yes/no) [no]:
 > 

 updated: vendor/mangopay/php-sdk-v2/MangoPay/Wallet.php

 Add another property? Enter the property name (or press <return> to stop adding fields):
 > 

  Success! 

 Next: When you're ready, create a migration with php bin/console make:migration

Obviously, I am expecting it to modify my entity, which is within the doctrine mapping. Using the full namespace the edit is performed as expected.

In this scenario it should be at least partially checked, and then possibly asked which class you're exactly referring to. The same might be the case when a FQCN name might cover up a local one through relations, as I believe I've seen something like this already.

jrushlow commented 5 months ago

Hmm ya, Maker shouldn't have anything todo with the vendor dir. I suspect this is happening either when we call createClassNameDetails() or when $generator->execute(). In either of those 2 paths, we use our autoloader to validate class names. Ultimately, we need to: 1) ~"global conditional" that throws an exception if $fs is attempting to CRUD $root_project_dir/vendor.~ 4 is a better idea 2) In our autoloader, exclude any classes that are

pkly commented 5 months ago

I think it should be checked if an alias exists as FQCN as well as in the mapping files (which are not read, afaik, since it's not possible to know what kind of setup people have?) and then provide a list of possible targets for classes, instead of picking one?

I think it should be possible to read Doctrine's ORM configuration after they've been processed, since the container has been built during maker's runtime, so maybe validate against that?

I've noticed a similar, but unrelated issue, as maker's config is very basic. It should be left in place for the time being, but I believe something like ORM should rely on the underlying framework configuration first (since, for example, we have multiple ORM mappings and it's a pain to switch maker between them), which would also allow for better validation in these types of cases.

pkly commented 2 months ago

Any idea if/when this will get looked at? I could look into it myself but it'll prolly take me way longer to familiarize myself with exactly what has to be done