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: add new view-string type #654

Merged
merged 6 commits into from
Sep 8, 2020
Merged

Conversation

Daanra
Copy link
Contributor

@Daanra Daanra commented Sep 7, 2020

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

This merge request introduces a new custom type to Larastan: blade-view.

A blade-view is any string that passes the view()->exists($string) test. I have updated the stubs to use this new type as well, such that one can no longer run into errors when trying to render a view that does not exist.

I'm not quite happy with the blade-view name. A CSS file will also pass the view()->exists() test, so I think the 'blade-' part is a bit misleading. However, simply view seems a bit ambiguous. I'm not sure what the best name for it is.

EDIT: Renamed blade-view to view-string.

Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

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

Hi,

Thank you! This sounds interesting. Although it adds more dynamic analysis to the codebase, which I don't quite like, it's ok I guess.

About the name of the new type. What about view-string? It's clear that it represents a view in a string form. Just a suggestion.

@canvural
Copy link
Collaborator

canvural commented Sep 8, 2020

Unrelated question: Why didn't StyleCI send a PR to fix the issues? 🤔

@szepeviktor szepeviktor marked this pull request as draft September 8, 2020 09:14
@Daanra Daanra changed the title WIP: Add new blade-view type WIP: Add new view-string type Sep 8, 2020
@Daanra Daanra changed the title WIP: Add new view-string type Add new view-string type Sep 8, 2020
@Daanra Daanra marked this pull request as ready for review September 8, 2020 19:17
@canvural
Copy link
Collaborator

canvural commented Sep 8, 2020

Thank you! LGTM!

Error in Travis seems unrelated to this. I'll fix that later.

@canvural canvural changed the title Add new view-string type feat: add new view-string type Sep 8, 2020
@canvural canvural merged commit 32ab011 into larastan:master Sep 8, 2020
@Daanra Daanra mentioned this pull request Sep 12, 2020
@gazben
Copy link

gazben commented Oct 26, 2020

@Daanra
One thing is not clear to me from the documentation. How to handle the case where I modify the view name?

Example:

    /**
     * @param string $view
     * @return \Illuminate\View\View
     */
    public function renderView(string $view): View
    {
        /**
         * @phpstan-param view-string $bladeView
         * @var string $bladeView
         */
        $bladeView = 'pages.' . $view;
        return view($bladeView);
    }

Output:

 ------ -----------------------------------------------------------------------------
  Line   Http\Controllers\DashboardController.php
 ------ -----------------------------------------------------------------------------
  28     Parameter #1 $view of function view expects view-string|null, string given.
 ------ -----------------------------------------------------------------------------

 [ERROR] Found 1 error

Did I miss something?

@Daanra
Copy link
Contributor Author

Daanra commented Oct 26, 2020

@gazben

Your code would work on lower levels of PHPStan, but on the maximum level it will throw an error because it's not 100% sure that it's a view-string. To fix this, you can type hint the $bladeView variable like you did. However, you used @phpstan-param instead of @phpstan-var, which is the reason why PHPStan is ignoring it, I believe.

This should work:

public function renderView(string $view): View
{
   /** @phpstan-var view-string $bladeView */
   $bladeView = 'pages.' . $view;
   return view($bladeView);
}

@gazben
Copy link

gazben commented Oct 26, 2020

It works. Thanks

@jordyvandomselaar
Copy link

This feature saved me a lot of time today. Thanks!

@bobbybouwmann
Copy link

@Daanra I think this is an awesome feature. However, the error message is very cryptic. It took me half an hour to figure out why I get this message in the first place.

 ------ -----------------------------------------------------------------------------
  Line   app/Http/Livewire/Tables/TurbinesTable.php
 ------ -----------------------------------------------------------------------------
  104    Parameter #1 $view of function view expects view-string|null, string given.
 ------ -----------------------------------------------------------------------------

It turns out that I renamed by the view and that's why I get this error, so that is good. However, the error doesn't describe that the view is not found. Why don't we show a message like View not found or something?

@Daanra
Copy link
Contributor Author

Daanra commented Nov 4, 2020

@bobbybouwmann I agree with you that the error message is not clear at all. The error message is generated by PHPStan's own type checking rules and it's not trivial to alter this message. It's an unfortunate downside to using custom Types in such a scenario. An alternative would be to define our own rules to perform the type checking (similar to #673), this would give us complete control over the error message, but requires quite a bit of implementation work.

@szepeviktor
Copy link
Collaborator

I agree with you too!

@szepeviktor
Copy link
Collaborator

szepeviktor commented Nov 4, 2020

Maybe adding it to the no-one-reads-docs will not help :)

@bobbybouwmann
Copy link

@Daanra Maybe renaming the type would already help. Something like missing-view-string. I believe this is the only place where this type check is used.

@szepeviktor
Copy link
Collaborator

Maybe renaming the type would already help. Something like missing-view-string. I

That is a really bad idea.

PHPStan is not the most user-friendly product, a bit esoteric.

@szepeviktor
Copy link
Collaborator

not the most user-friendly product,

But @phpstan is my favorite software!

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.

6 participants