Skip to content

feat: make:entity php 8 attributes support #926

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

Closed
wants to merge 10 commits into from
Closed

feat: make:entity php 8 attributes support #926

wants to merge 10 commits into from

Conversation

Geekimo
Copy link
Contributor

@Geekimo Geekimo commented Jul 13, 2021

Based on #920

  • Updated tests
  • Re-added deleted annotated class detection, and added attribute mapping detection.

Copy link

@php-programmist php-programmist left a comment

Choose a reason for hiding this comment

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

Not working for me.
I have installed package gesdinet/jwt-refresh-token-bundle wich has annotated entity. And I have next situation:
substr_compare('App\Entity\Test', 'App\Entity', 0);//5
substr_compare('App\Entity\Test', 'Gesdinet\JWTRefreshTokenBundle\Entity', 0);//-6
And AnnotationDriver returns instead of AttributeDriver


foreach ($this->annotatedPrefixes as $key => $mappings) {
foreach ($mappings as [$prefix, $driver]) {
$diff = substr_compare($namespace, $prefix, 0);

Choose a reason for hiding this comment

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

This works incorrect. I have installed package gesdinet/jwt-refresh-token-bundle wich has annotated entity. And I have next:
substr_compare('App\Entity\Test', 'App\Entity', 0);//5
substr_compare('App\Entity\Test', 'Gesdinet\JWTRefreshTokenBundle\Entity', 0);//-6
And AnnotationDriver returns instead of AttributeDriver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi !
This behavior sounds logical, as the purpose of this part of the code is to return the driver used for a mappping.
So for Gesdinet\JWTRefreshTokenBundle\Entity, which is annotated, the fact that you have a AnnotationDriver is correct.
Am I missing something ?

Copy link

@php-programmist php-programmist Jul 31, 2021

Choose a reason for hiding this comment

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

I have 2 prefixes (name spaces):

  1. App\Entity - Attribute Driver
  2. Gesdinet\JWTRefreshTokenBundle\Entity - Annotation Driver

I executed next command:
php bin/console make:entity Test
I expected that would be created App\Entity\Test entity with attributes, but I got - annotations!
I figured out that method getMappingDriverForNamespace returns incorrect Driver. Now this method returns the last driver of $this->annotatedPrefixes array, not the closest match.
I see 2 problems in mentioned block:

  1. $lowestCharacterDiff always null. And if statement (row 325) always true
  2. For entity App\Entity\Test closest match must be App\Entity.

But
substr_compare('App\Entity\Test', 'App\Entity', 0); results as 5
substr_compare('App\Entity\Test', 'Gesdinet\JWTRefreshTokenBundle\Entity', 0); results as -6
-6 is less than 5, and this algorithm choosing incorrect driver
In my fork I using next variant of if block:

if (null === $lowestCharacterDiff || ($diff < $lowestCharacterDiff && $diff > 0)) {
      $lowestCharacterDiff = $diff;
      $foundDriver = $driver;
}

I am not absolutely sure that it is correct, but it work for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, your first message wasn't that explicit.
I'm gonna check differently this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This point has been fixed by @adlpz in #920

@fabpot
Copy link
Member

fabpot commented Sep 29, 2021

This one can be closed I suppose.

@jrushlow
Copy link
Collaborator

Good catch @fabpot.

Feature consolidated in PR #978 from this PR & PR #920. Thanks @Geekimo for your contribution to this awesome new feature!

@jrushlow jrushlow closed this Sep 29, 2021
@jrushlow jrushlow added the Feature New Feature label May 10, 2022
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.

4 participants