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

feat(phpstan): foundation for usage in extensions #3666

Merged
merged 12 commits into from
Jan 15, 2023
Merged

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Nov 3, 2022

Changes proposed in this pull request

In #3553, we added phpstan static code analysis, but only to core code. This PR aims to make the flarum/phpstan package usable by extension repositories. Which means (at the moment) being able to pick up on new model relations and attributes.

Static analysis is actually expanded to other extensions in a separate PR for ease of readability (#3667).

There are two crucial changes/additions introduced here.

Model::cast() Extender

A new cast(string $name, string $type) extender is introduced, which allows adding type casting to attributes added to existing models from extensions. This provides three advantages:

  • The feature itself is a good addition to extensions.
  • Our phpstan package can tell that an attribute added to a core model from an extension is a valid class property, since those are accessed though magic methods.
  • Deprecates the old dateAttribute extender, since laravel has deprecated the use of the $dates property in models for date attributes, and recommends using $casts instead. I hope in another PR to also change model classes from directly using $dates property.

Extending PHPStan

This PR also extends phpstan so that it can tell from extend.php files what relations and attributes are added to existing models, so that they aren't reported as unknown. extend.php files are parsed and currently the package looks up calls to cast and hasOne belongsTo hasMany belongsToMany method calls to inform the phpstan utility about them.

Reviewers should focus on:

  • cast model extender.
  • Deprecating dateAttribute extender.
  • Generally PHPStan extension logic, I would focus less on the code for it.

Follow-up Tasks

  • Write documentation about using PHPStan in extensions.
  • Add PHPStan infrastructure to Flarum CLI.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@SychO9 SychO9 self-assigned this Nov 3, 2022
@SychO9 SychO9 requested a review from a team as a code owner November 3, 2022 19:50
@SychO9 SychO9 changed the title Sm/phpstan in extensions feat(phpstan): foundation for usage in extensions* Nov 3, 2022
@SychO9 SychO9 changed the title feat(phpstan): foundation for usage in extensions* feat(phpstan): foundation for usage in extensions Nov 3, 2022
@SychO9 SychO9 linked an issue Nov 4, 2022 that may be closed by this pull request
SychO9 and others added 11 commits January 13, 2023 20:06
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Stops using `dates` as it's deprecated in laravel 8

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 added this to the 1.7 milestone Jan 14, 2023
@SychO9 SychO9 merged commit 5fe3cfd into main Jan 15, 2023
@SychO9 SychO9 deleted the sm/phpstan-in-extensions branch January 15, 2023 14:25
private function findCastAttributeMethod(ClassReflection $classReflection, string $propertyName): ?MethodCall
{
foreach ($this->extendersResolver->getExtenders() as $extender) {
if (! $extender->isExtender('Model')) {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe this string would make sense to put in a constant somewhere? Or could we reflect the model itself to get the unqualified class name?

}

foreach (array_merge([$classReflection->getName()], $classReflection->getParentClassesNames()) as $className) {
if ($className === 'Flarum\Database\AbstractModel') {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Same here, might be cleaner to put all these strings in one place. Deriving it from the class would be nice, but I assume impossible since then phpstan ext would need to depend on core

use PHPStan\Type\ObjectType;
use PHPStan\Type\UnionType;

class ModelDateAttributesExtension implements PropertiesClassReflectionExtension
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

There's a lot of code repetition; maybe we should consider some abstract class on top of PropertiesClassReflectionExtension for finding extenders?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can further iterate on this 👍🏼

$this->paths = $paths;
}

public function getExtenderFiles(): array
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@luceos luceos mentioned this pull request Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHPStan Setup
3 participants