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

ignore phpstorm attributes when instantiating and add readonly property #281

Merged
merged 21 commits into from
Dec 21, 2022

Conversation

jhavenz
Copy link

@jhavenz jhavenz commented Dec 1, 2022

This package throws an error while instantiating a data class if/when a built-in php storm attribute is used on a dto class.
e.g.:

#[\JetBrains\PhpStorm\Immutable]
class PhpStormClassAttributeData extends Data
{
    public function __construct(public string $property)
    {
        $this->property = $property;
    }
}

and/or

class PhpStormClassAttributeData extends Data
{
    #[\JetBrains\PhpStorm\Immutable]
    public string $property;

    public function __construct(string $property)
    {
        $this->property = $property;
    }
}

and/or

class PhpStormClassAttributeData extends Data
{
    public function __construct(
       #[\JetBrains\PhpStorm\Immutable]
       public string $property
    ) {
       //
    }
}

I'm using \JetBrains\PhpStorm\Immutable in these examples, but I've written this PR to ignore any of the other attributes that they have within the same namespace


In the process of fixing this functionality, I came across the need to add the a 'isReadonly' property to the DataType class ... I was actually surprised it wasn't already there. Though, despite me using this package (and source-diving) it quite a bit, maybe its elsewhere in the library and I'm just unaware. Just lmk.

Love the package, thx for the great work!

patinthehat and others added 9 commits November 25, 2022 08:24
Bumps [ramsey/composer-install](https://github.com/ramsey/composer-install) from 1 to 2.
- [Release notes](https://github.com/ramsey/composer-install/releases)
- [Commits](ramsey/composer-install@v1...v2)

---
updated-dependencies:
- dependency-name: ramsey/composer-install
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 3.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2...v3)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
…amsey/composer-install-2

Bump ramsey/composer-install from 1 to 2
…ctions/checkout-3

Bump actions/checkout from 2 to 3
add missing `array $context` to cast method
@jhavenz jhavenz force-pushed the feature/ignore-phpstorm-attributes branch from 2acbe70 to 3caed1b Compare December 1, 2022 06:40
Copy link
Member

@rubenvanassche rubenvanassche left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, I've left a few remarks + could you also provide tests for the readonly functionality. And is it possible to add support for PHP 8.2 readonly classes? So we can get it in one go!

Thanks!

src/Support/DataClass.php Outdated Show resolved Hide resolved
src/Support/DataProperty.php Outdated Show resolved Hide resolved
src/Support/DataType.php Outdated Show resolved Hide resolved
@jhavenz
Copy link
Author

jhavenz commented Dec 11, 2022

Ok, no problem. Your comments make good sense to me. Ill get them fixed up.

@jhavenz
Copy link
Author

jhavenz commented Dec 12, 2022

@rubenvanassche
While working on your requested changes, I've realized that checking if a Data class is readonly is somewhat irrelevant since the abstract \Spatie\LaravelData\Data class itself is not readonly - and as of now, 8.2 won't allow readonly classes to extend non-readonly classes.
At some point, you may want to consider splitting the abstract \Spatie\LaravelData\Data class into 2 base classes, so they can then be extended for readonly/non-readonly purposes. But as of now, I currently have the $isReadonly property implemented on both the DataClass and the DataProperty class.

You think I should just keep these properties as they are, though the DataClass::isReadonly property won't really get used? I'm also not able to test the DataClass::isReadonly property for these same reasons.
Or, just keep the $isReadonly property or on the DataProperty since it's the only one being used?

@jhavenz jhavenz force-pushed the feature/ignore-phpstorm-attributes branch 3 times, most recently from 72fb0d2 to 5eb1960 Compare December 12, 2022 20:58
@jhavenz jhavenz force-pushed the feature/ignore-phpstorm-attributes branch from 5eb1960 to 689703a Compare December 12, 2022 20:59
@jhavenz
Copy link
Author

jhavenz commented Dec 12, 2022

Ok, I've pushed up the changes I've made. If you decide you'd like to keep the DataClass::isReadonly property, this PR is good to go with the changes you've requested.

@rubenvanassche
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants