laminas / laminas-navigation

Manage trees of pointers to web pages in order to build navigation systems
https://docs.laminas.dev/laminas-navigation/
BSD 3-Clause "New" or "Revised" License
17 stars 11 forks source link

Improved type inference around page containers, upgraded `vimeo/psalm` and dependencies #40

Closed renovate[bot] closed 1 year ago

renovate[bot] commented 1 year ago

Mend Renovate

This PR contains the following updates:

Update Change
lockFileMaintenance All locks refreshed

πŸ”§ This Pull Request updates lock files to use the latest dependency versions.


Configuration

πŸ“… Schedule: Branch creation - "before 2am" in timezone UTC, Automerge - At any time (no schedule defined).

🚦 Automerge: Enabled.

β™» Rebasing: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

πŸ‘» Immortal: This PR will be recreated if closed unmerged. Get config help if that's undesired.



Read more information about the use of Renovate Bot within Laminas.

renovate[bot] commented 1 year ago

Branch automerge failure

This PR was configured for branch automerge. However, this is not possible, so it has been raised as a PR instead.


renovate[bot] commented 1 year ago

Edited/Blocked Notification

Renovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR. You can manually request rebase by checking the rebase/retry box above.

⚠ Warning: custom changes will be lost.

Ocramius commented 1 year ago

Type inference improved: from 92.9590% to 93.4685%

MidnightDesign commented 1 year ago

@Ocramius I don't know about Psalm, but this completely breaks PHPStan. Say I have a (native) return type AbstractPage somewhere. PHPStan now complains about the missing type argument. So I add a return tag @returns AbstractPage<AbstractPage> to the method - no problem, right? Well, now PHPStan wants a type argument for the inner AbstractPage and so on.

Am I just missing something here?

Ocramius commented 1 year ago

PHPStan supports templates too: do you have an example, perhaps an error reported by phpstan?

Be aware that we keep parity with psalm, for now, so if psalm is ahead, phpstan will indeed report issues.

Ocramius commented 1 year ago

What you probably want:

@template TPage of AbstractPage
@template-extends AbstractPage<TPage>
MidnightDesign commented 1 year ago

I'm sorry, I probably wasn't being clear enough. I am well aware of PHPStan's template support. I also use templates a lot.

Here's an example:

    // We're in some page factory class

    public function createPage(): AbstractPage
    {
        // ...
    }

PHPStan complains about the missing type argument for the returned AbstractPage:

Method MyFactory::createPage() return type with generic class Laminas\Navigation\Page\AbstractPage does not specify its types: TPage

When I add the type argument:

    /**
     * @return AbstractPage<AbstractPage>
     */

PHPStan complains about the missing type argument for the "inner" AbstractPage. So it basically wants AbstractPage<AbstractPage<AbstractPage<AbstractPage<...>>>>.

I guess what you're saying is "make your page factory class generic as well"? Well, this factory isn't the only place I'm using pages. You've got the same problem if you just have a private AbstractPage $page property somewhere. I would have to make all classes and interfaces generic that use pages in some way. Also, that would just kick the bucket down the road, right?

I think this all boils down to the lack of default types for type arguments like you have in TypeScript:


interface Foo<T extends object = {myDefault: string}> {
    // ...
}
internalsystemerror commented 1 year ago

I mainly use PHPStan over psalm and they're pretty similar. You can use a template in the method instead of the class if you want to avoid making your factories generic too. Though I have to ask why the solution to this would be to weaken the types in laminas and not strengthen the types in the implementation?

MidnightDesign commented 1 year ago

@internalsystemerror You're right, I haven't thought of making the methods themselves generic. That would be a solution, thanks.

I'm not saying we should weaken Laminas' types, I was just stating my problem and asking for a solution. Believe me, my collegues sometimes hate me for being so crazy about strict typing.

But in this particular case I really don't see any benefit in Mvc, AbstractPage and Navigation being generic. They could just as well @extends AbstractContainer<AbstractPage> and be done with it. Making them generic forces you to add the docblock type annotation anywhere you're referencing those symbols without any obvious benefit. I don't care if the thing contains Mvcs, Uris or custom implementations of AbstractPage.

MidnightDesign commented 1 year ago

I just tried adding a @template TPage of AbstractPage @return AbstractPage<TPage> to the factory method, but that doesn't work because the type argument must be referenced in the method's in a parameter (but it's only referenced in the return type).

internalsystemerror commented 1 year ago

@MidnightDesign ah fair ... Mvc and Navigation being generic means you can't use those in place of AbstractPage to solve the problemπŸ‘. I'm not sure what an implementation should look like and I'm starting to think maybe AbstractPage shouldn't be generic at all. If the purpose for doing so is for child pages, they could also be any mix of classes that extend AbstractPage so the generic template would most likely always have to be AbstractPage.

MidnightDesign commented 1 year ago

@internalsystemerror Exactly.

@Ocramius What do you think? I can open a PR if you want.

Ocramius commented 1 year ago

I'm not following: send a patch with your idea, and let's discuss on a diff.