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

Update stubs for new model-property type #676

Open
canvural opened this issue Oct 7, 2020 · 7 comments
Open

Update stubs for new model-property type #676

canvural opened this issue Oct 7, 2020 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@canvural
Copy link
Collaborator

canvural commented Oct 7, 2020

The Problem

With the new model-property type and ModelPropertyRule Larastan now has the ability to check the existence of model columns that are given to a method.

Adding the model-property type to the Laravel's core methods are done with the help of PHPstan stubs. Some examples can be seen here. Currently, not all the Laravel methods that are doing something with the model properties are updated in stubs. And we need help with updating this.

Contributing

You can contribute to this issue by updating the stubs and tests. Below you can read the detailed instructions for how to that. In case you have any questions you can ask them in this issue, or you can reach out to me at Twitter

Preparation

Helpful guides

Updating stubs

Here is the directory containing all of the stubs Larastan uses. All of the stubs that need updating are already there, so you don't need to create new files. Just updating method definitions inside.

Steps

  1. Find a Laravel method that does not exist in our stubs, or exists but the parameter type is not yet updated. For this example, we will use this method here: https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/Eloquent/Builder.php#L511

  2. Then we need to copy the method definition into our stub. So in this file, at the end we can add the following code:

/**
 * Get a single column's value from the first result of a query.
 *
 * @param  model-property<TModelClass>  $column
 * @return mixed
 */
public function value($column);

Here we used generic type model-property<TModelClass> because we only want to allow the model properties from the current model. You can see the TModelClass defined at the beginning of the class. If the stub you are updating does not have any @template definition for the class you can just use model-property

  1. As a last step we need to update the tests to verify our changes are working as we expect.
    The method we added to stubs was on Eloquent Builder class, so we need to add our test case to the model-property-builder file. If the stu you added was in one of the relation classes you can use model-property-relation file. If it was in the model class itself, you can use model-property-model file.

Our test case can be as simple as the following:

\App\User::query()->value('emaiil');

We intentionally made a typo and we expect Larastan to catch it.

Note: Here you can also add test cases that shouldn't produce any error. If you want to test that aspect of the method too.

Now that we added a test case, we also need to add an assertion that the expected error is produced. For that we need to add a new line to the testModelPropertyRuleOnBuilder method in ModelPropertyRuleTest class.

public function testModelPropertyRuleOnBuilder(): void
{
    $errors = $this->setConfigPath(__DIR__.DIRECTORY_SEPARATOR.'Data/modelPropertyConfig.neon')->findErrorsByLine(__DIR__.'/Data/model-property-builder.php');

    self::assertEquals([
        // Rest of the assertions
        35 => 'Property \'emaiil\' does not exist in App\\User model.', // Newly added line
    ], $errors);
}

Here the array key represents the line number of the error. And the value is the message we expect to be produced.

Now let's run the whole test suit to see we did not break anything else. You can do that by running the command composer test in your terminal. If everything goes well you should see all green:

image

Then you can commit your changes, push it to your fork of the repo then go onto Github and open a pull request!

That's it! You've successfully created a PR on an open-source project 🥳

(PS: The example used in this issue is not currently in the stubs. So you can just follow the guide, copy the code and make an easy first contribution 😊)

List of PRs

@canvural canvural added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Oct 7, 2020
@szepeviktor szepeviktor pinned this issue Oct 7, 2020
@xitox97
Copy link

xitox97 commented Oct 7, 2020

Hi @canvural, thank you for writing this awesome instruction. It is really helpful for beginners like me.

I have a question. Does 1 PR can only contains 1 method or 1 PR can contains multiple method?

@szepeviktor
Copy link
Collaborator

Thank you @xitox97 for your intention.
You may send as many stubs as you want in one PR.

@canvural
Copy link
Collaborator Author

canvural commented Oct 7, 2020

@xitox97 Yeah it is ok to add more than one method in PR 👍

@robbplo
Copy link
Contributor

robbplo commented Oct 8, 2020

I've added two stubs in this PR: #679

I didn't link the issue so it wouldn't close if it gets merged, because there is more work to be done!
Let me know if there's some changes I still need to make

@xitox97
Copy link

xitox97 commented Oct 10, 2020

Edit: Issue #681 👀

@szepeviktor
Copy link
Collaborator

Hello @xitox97! Please open a proper issue.

@xitox97
Copy link

xitox97 commented Oct 10, 2020

Hello @xitox97! Please open a proper issue.

Ops sorry, I thought can just ask it here. I open the issue here #681

@canvural canvural unpinned this issue Nov 25, 2020
@canvural canvural changed the title [Hacktoberfest 2020] Update stubs for new model-property type Update stubs for new model-property type Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants