maker-bundle icon indicating copy to clipboard operation
maker-bundle copied to clipboard

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

Open pkly opened this issue 1 year ago • 3 comments

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.

pkly avatar May 23 '24 07:05 pkly

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
    • Outside of the maker.root_namespace NS (defaults to App)
  3. This should effectively limit $fs to only CRUD php files within the maker.root_ namespace.
  4. Throw an exception if makers $fs is trying to modify php files outside of the src/ / maker.root_namespace dir paths.

jrushlow avatar May 23 '24 10:05 jrushlow

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 avatar May 23 '24 10:05 pkly

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

pkly avatar Aug 20 '24 08:08 pkly