Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Open
pkly opened this issue May 23, 2024 · 3 comments
Open

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

pkly opened this issue May 23, 2024 · 3 comments
Labels
Bug Bug Fix

Comments

@pkly
Copy link

pkly commented May 23, 2024

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 pkly changed the title [make:entity] Maker modifies vendor files if namespace matches [make:entity] Maker modifies vendor files if FQCN covers up local alias May 23, 2024
@jrushlow jrushlow added the Bug Bug Fix label May 23, 2024
@jrushlow
Copy link
Collaborator

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.

@pkly
Copy link
Author

pkly commented May 23, 2024

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
Copy link
Author

pkly commented Aug 20, 2024

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug Bug Fix
Projects
None yet
Development

No branches or pull requests

2 participants