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

Entities: Allow FieldItemList access through magic __get/__set methods #27

Merged
merged 4 commits into from May 3, 2019

Conversation

Projects
None yet
3 participants
@Fonata
Copy link
Contributor

commented Feb 2, 2019

PHPStan said "Access to an undefined property". A custom entity may
define fields in ::baseFieldDefinitions(). Other ways to define a
field are though hook implementations like hook_entity_base_field_info(),
or through config entites created from the Drupal admin UI. Thus,
it seems very hard to track all cases in PHPStan.

While ContentEntityBase::__get() will return a FieldItemListInterface,
ContentEntityBase::__set may be used to set fields directly, thus I
decided to tell PHPStan that the type is mixed. For example, the following
code can be valid:

$entity = SomeCustomEntity();
$entity->some_int_field = 34;
$entity->save();
@mglaman

This comment has been minimized.

Copy link
Owner

commented Feb 3, 2019

This is a hard problem. Ideally I would rather see dynamic properties reflected as FieldItemListInterface, since that is what they are.

We could assume it is FieldItemList which has __get and __set to work around the ::first() method. But we technically cannot.

@Fonata

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

@mglaman Thanks for your time and feedback.

Should I go ahead and make Travis happy? I will if you agree with the general approach.

Show resolved Hide resolved src/Reflection/EntityFieldsViaMagic.php Outdated
Show resolved Hide resolved src/Reflection/EntityFieldsViaMagic.php Outdated
@Fonata

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

I messed one of the commits up: I had no intention of changing DrupalExtension.php, so I added a fixup commit. Git will fix things for us when running git rebase --autosquash.

@Fonata

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

@mglaman When you find the time: Please let me know whether we're moving in the right direction. Once I know the code here is stable enough, I will try to add PHPUnit test coverage. Thanks.

@Fonata Fonata force-pushed the Fonata:content-entity-fields-magic branch 2 times, most recently from 788e817 to 86a6d23 Feb 24, 2019

@Fonata

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

I just rebased this to the current master branch as there were conflicts.

If there is anything else I can do: just let me know.

@mglaman

This comment has been minimized.

Copy link
Owner

commented Mar 8, 2019

@Fonata sorry I've been busy. I'm working to make time to come back over here.

@Fonata

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

No problem at all, your pace is fine.

@mglaman

This comment has been minimized.

Copy link
Owner

commented May 2, 2019

:D We have test coverage! Let's jam on this.

@mglaman mglaman self-requested a review May 2, 2019

@mglaman

This comment has been minimized.

Copy link
Owner

commented May 2, 2019

@Fonata sorry I screwed up the branch a bit, but the diff is OK enough for a squash merge. Working on the tests.

@mglaman

mglaman approved these changes May 3, 2019

@mglaman mglaman self-assigned this May 3, 2019

@mglaman

This comment has been minimized.

Copy link
Owner

commented May 3, 2019

With the test and without the broker addition the output should be:

        $label1 = $testEntity->label(); // 17
        $label2 = $testEntity->get('name')->first()->value; // 18
        $label3 = $testEntity->name->first()->value; // 19
        $label4 = $testEntity->name->value; // 20
Array
(
    [0] => PHPStan\Analyser\Error Object
        (
            [message:PHPStan\Analyser\Error:private] => Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$value.
            [file:PHPStan\Analyser\Error:private] => /Users/mglaman/Projects/php/phpstan-drupal/tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldFixture.php
            [line:PHPStan\Analyser\Error:private] => 18
            [canBeIgnored:PHPStan\Analyser\Error:private] => 1
        )

    [1] => PHPStan\Analyser\Error Object
        (
            [message:PHPStan\Analyser\Error:private] => Access to an undefined property Drupal\entity_test\Entity\EntityTest::$name.
            [file:PHPStan\Analyser\Error:private] => /Users/mglaman/Projects/php/phpstan-drupal/tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldFixture.php
            [line:PHPStan\Analyser\Error:private] => 19
            [canBeIgnored:PHPStan\Analyser\Error:private] => 1
        )

    [2] => PHPStan\Analyser\Error Object
        (
            [message:PHPStan\Analyser\Error:private] => Access to an undefined property Drupal\entity_test\Entity\EntityTest::$name.
            [file:PHPStan\Analyser\Error:private] => /Users/mglaman/Projects/php/phpstan-drupal/tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldFixture.php
            [line:PHPStan\Analyser\Error:private] => 20
            [canBeIgnored:PHPStan\Analyser\Error:private] => 1
        )

)
@mglaman

This comment has been minimized.

Copy link
Owner

commented May 3, 2019

With the extension added to the broker:

        $label1 = $testEntity->label(); // 17
        $label2 = $testEntity->get('name')->first()->value; // 18
        $label3 = $testEntity->name->first()->value; // 19
        $label4 = $testEntity->name->value; // 20

It fixed line 20! which is what we intended to do, here.

Array
(
    [0] => PHPStan\Analyser\Error Object
        (
            [message:PHPStan\Analyser\Error:private] => Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$value.
            [file:PHPStan\Analyser\Error:private] => /Users/mglaman/Projects/php/phpstan-drupal/tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldFixture.php
            [line:PHPStan\Analyser\Error:private] => 18
            [canBeIgnored:PHPStan\Analyser\Error:private] => 1
        )

    [1] => PHPStan\Analyser\Error Object
        (
            [message:PHPStan\Analyser\Error:private] => Access to an undefined property Drupal\Core\TypedData\TypedDataInterface::$value.
            [file:PHPStan\Analyser\Error:private] => /Users/mglaman/Projects/php/phpstan-drupal/tests/fixtures/drupal/modules/phpstan_fixtures/src/EntityFieldFixture.php
            [line:PHPStan\Analyser\Error:private] => 19
            [canBeIgnored:PHPStan\Analyser\Error:private] => 1
        )

)
@mglaman
Copy link
Owner

left a comment

There are some nit picks I want to address. I also want to spend some time running through the test with xdebug to catch anything glaring.

Show resolved Hide resolved src/Reflection/EntityFieldReflection.php
Show resolved Hide resolved src/Reflection/EntityFieldReflection.php Outdated
Show resolved Hide resolved src/Reflection/FieldItemListPropertyReflection.php Outdated
Show resolved Hide resolved src/Reflection/FieldItemListPropertyReflection.php Outdated
Show resolved Hide resolved src/Reflection/FieldItemListPropertyReflection.php
Show resolved Hide resolved src/Reflection/FieldItemListPropertyReflection.php Outdated
@hannsen

This comment has been minimized.

Copy link

commented May 3, 2019

Lol sorry, I wrote a review like a month ago, but forgot to submit it. Not used to Github Reviews

Entities: Allow FieldItemList access through magic __get/__set methods
PHPStan said "Access to an undefined property". A custom entity may
define fields in ::baseFieldDefinitions(). Other ways to define a
field are though hook implementations like hook_entity_base_field_info(),
or through config entites created from the Drupal admin UI. Thus,
it seems very hard to track all cases in PHPStan.

While ContentEntityBase::__get() will return a FieldItemListInterface,
ContentEntityBase::__set may be used to set fields directly, thus I
decided to tell PHPStan that the type is mixed. For example, the following
code can be valid:

````php
$entity = SomeCustomEntity();
$entity->some_int_field = 34;
$entity->save();
````

@mglaman mglaman force-pushed the Fonata:content-entity-fields-magic branch from a328dcf to a534cbf May 3, 2019

Fonata and others added some commits Feb 5, 2019

Improved the types this entity extension works on
Also:
- Added support for 4 certain special properties original, target_id
  entity and value.
- Split the inline class into separate files for better maintainability.

Remove the need for the Drupal autoloader to work

This allows Travis' PHPStan CLI to check this extention.

@mglaman mglaman force-pushed the Fonata:content-entity-fields-magic branch from a534cbf to 35e9765 May 3, 2019

@mglaman

mglaman approved these changes May 3, 2019

@mglaman mglaman merged commit ebf69ca into mglaman:master May 3, 2019

5 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test_drupal Your tests passed on CircleCI!
Details
ci/circleci: test_drupal_project Your tests passed on CircleCI!
Details
@mglaman

This comment has been minimized.

Copy link
Owner

commented May 3, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.