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

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

Merged
merged 11 commits into from
Dec 3, 2022

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Dec 3, 2022

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.


  • If you want to rebase/retry this PR, check this box

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

Signed-off-by: Renovate Bot <bot@renovateapp.com>
| datasource | package     | from   | to    |
| ---------- | ----------- | ------ | ----- |
| packagist  | vimeo/psalm | 4.30.0 | 5.1.0 |


Signed-off-by: Renovate Bot <bot@renovateapp.com>
@renovate renovate bot added the renovate label Dec 3, 2022
@renovate
Copy link
Contributor Author

renovate bot commented Dec 3, 2022

Branch automerge failure

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


  • Branch has one or more failed status checks

The constraint on `laminas/laminas-servicemanager:^3` has been added ages ago: this code is dead
and defunct.
The constraint on `laminas/laminas-servicemanager:^3` has been added ages ago: this code is dead
and defunct.
@Ocramius Ocramius mentioned this pull request Dec 3, 2022
1 task
@renovate
Copy link
Contributor Author

renovate bot commented Dec 3, 2022

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
Copy link
Member

Ocramius commented Dec 3, 2022

Type inference improved: from 92.9590% to 93.4685%

src/AbstractContainer.php Outdated Show resolved Hide resolved
src/Page/AbstractPage.php Show resolved Hide resolved
@Ocramius Ocramius changed the title Lock file maintenance Improved type inference around page containers, upgraded vimeo/psalm and dependencies Dec 3, 2022
@Ocramius Ocramius self-assigned this Dec 3, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

🚢 here meanwhile - rolling a release too :)

@Ocramius Ocramius merged commit 85edc8a into 2.17.x Dec 3, 2022
@Ocramius Ocramius deleted the renovate/lock-file-maintenance branch December 3, 2022 21:08
@MidnightDesign
Copy link
Contributor

@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
Copy link
Member

Ocramius commented Dec 6, 2022

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
Copy link
Member

Ocramius commented Dec 6, 2022

What you probably want:

@template TPage of AbstractPage
@template-extends AbstractPage<TPage>

@MidnightDesign
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

@internalsystemerror Exactly.

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

@Ocramius
Copy link
Member

Ocramius commented Dec 6, 2022

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

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.

None yet

3 participants