Fix incorrect @mixin in PendingAwaitablePage

[DrWarpMan]

Issue

I believe the @mixin directive inside PendingAwaitablePage class is currently wrong.

PendingAwaitablePage makes calls to AwaitableWebpage, not the Webpage class.
It's the AwaitableWebpage that makes calls to Webpage afterwards.

As far as I can see the order is: PendingAwaitablePage => AwaitableWebpage => Webpage

The currently incorrect @mixin, is the reason for the following issue with VS Code PHP Intelephense extension. PHPStorm on the other hand seems to consider union mixin as intersection, so the problem does not occur there.

Solution

AwaitableWebpage does have @mixin Webpage, so we can just remove the Webpage from the mixin in PendingAwaitablePage class, since it's redundant anyway.

[nunomaduro]

sorry, i was not able to understand the issue, can you re-create it and elaborate?

[DrWarpMan]

@nunomaduro the issue is, I don't get typings/autocomplete in VS code with the PHP Intelephense extension.
And the reason for that is the @mixin inside the PendingAwaitablePage class.

When you add @mixin A|B to a class, it means, the class will have either A methods, or B methods, not both.

If you wanted to do both (intersection), you'd write them on two separate lines:

/**
 * @mixin A
 * @mixin B
 */
class MixedClass

And I believe PendingAwaitablePage is extending both Webpage & AwaitableWebpage, which is an intersection, not "one or the other".

So in the pest plugin browser code, you can either do:

/**
 * @mixin AwaitableWebpage
 * @mixin Webpage
 */
 class PendingAwaitablePage

Or remove Webpage from it altogether, hence AwaitableWebpage already uses @mixin Webpage.

/**
 * @mixin AwaitableWebpage
 */
 class PendingAwaitablePage

Here is a PHPStan playground example with @mixin union. (errors) ❌
Here is a PHPStan playground example with @mixin intersection. (ok) ✅

Note: Even if we didn't consider my specific IDE problem with this, I think the Webpage mixin is still wrong inside the PendingAwaitablePage class and can be safely removed, cause it is already defined within the AwaitableWebpage class.

[DrWarpMan]

VS code PHP Intelephense extension developers made a change on their side, so this is no longer an issue for me anyways.

[ace-of-aces]

@DrWarpMan

I just faced this issue too with Intelephense, but I don't think Pest's source code is at fault here.

The PHPStan playground you linked is expected to produce an issue because class B doesn't have the foo method defined, and not because the @mixin union syntax is invalid.

Here's a working version: https://phpstan.org/r/e313887f-0cb6-4d7a-b787-059f2eea157e

As Pest relies on this heavily internally and it passes in PHPStan, I'd suggest we'd reopen this issue in Intelephense or creating another one.

[ace-of-aces]

VS code PHP Intelephense extension developers laravel/vs-code-extension#532, so this is no longer an issue.

I created this PR, and it's still open as you may have noticed. I'm not a maintainer there, so I don't know if it will be merged😅

And more importantly, that PR won't solve this issue. It would just enable the generation of a helper file which patches the $this variable inside of tests, and helps with custom expectations, but it isn't concerned with pest-plugin-browser.

[DrWarpMan]

I created this PR, and it's still open as you may have noticed. I'm not a maintainer there, so I don't know if it will be merged😅

And more importantly, that PR won't solve this issue. It would just enable the generation of a helper file which patches the $this variable inside of tests, and helps with custom expectations, but it isn't concerned with pest-plugin-browser.

I used the pre-release version of Intelephense and I no longer had problems there,
also both of our issues at the Intelephense repo was set to "completed" too,
and I could only find a reference to your PR, so I thought your PR was already in, or at least in the pre-release version?

EDIT: Ah I understand now.. Intelephense is closed source 😅, and the PR I linked wasn't even in Intelephense, sorry I got it all confused.

Anyways, for the matter at hand:

The PHPStan playground you linked is expected to produce an issue because class B doesn't have the foo method defined, and not because the @mixin union syntax is invalid.

But that's the point. That's exactly what is happening in Pest code.
Here is a stripped 1:1 version of Pest's code with the PHPStan error.

From the information I gathered, I believe this is what is happening:
The syntax @mixin A|B, is a union of two classes, not intersection.
That means, the class using this mixin will either have methods of A, or B, not both.

Solution?
1.) Remove redundant Webpage mixin
This is how it's supposed to be anyway. This effectively creates the following inheritance:
PendingAwaitablePage extends AwaitableWebpage extends Webpage
Which is exactly what the code is doing, within those classes, via magic functions and whatnot.

2.) Use a mixin intersection
This works too, but we will end up having @mixin Webpage written twice in the code, which is redundant.
So I'd go with solution 1.

[ace-of-aces]

@DrWarpMan
Thx for the explanation!

Yeah, seems like it's not really correct that the mixin union is being used in this case..

Ah I understand now.. Intelephense is closed source 😅, and the PR I linked wasn't even in Intelephense, sorry I got it all confused

All good, yeah I had an issue in Intelephense with the @param-closure-this tag that was related to the mixin issue, which linked to my PR to the Laravel VS Code extension :)

I just switched to the pre-release version of Intelephense too, great that it seems to be solved!🎉

That said, as you explained that the mixin tags here are not really correct, and I'd support you in opening another PR, otherwise I'm glad that the end result seems to work now at least :)