🇦🇺Australia @mstrelan

Added MR for 11.0.x

We can probably add

protected bool $useOneTimeLoginLinks = FALSE;

to the failing test.

The array to string conversion is bogus, it doesn't work with the strlen we had before so it doesn't need to work now.

I think we need to get a list of their roles and see if any of them are "isAdminRole". I wonder if there is a helper function to find this for a user. If not, maybe there should be.

I think we need to get a list of their roles and see if any of them are "isAdminRole". I wonder if there is a helper function to find this for a user. If not, maybe there should be.

Nice timing, I was just finishing up the tests. What do you think of these changes?

Thanks for looking at this @bbrala. I initially went down the simple arrow function route:

fn ($value) => strlen($value) > 0

But strlen is coercing values to a string. If we later add declare(strict_types=1) then we get a TypeError. So we'd need to cast to match the current logic:

fn ($value) => strlen((string) $value) > 0

But later we might decide arrow functions should be typed:

fn (mixed $value): bool => strlen((string) $value) > 0

We might also later decide to enable SlevomatCodingStandard.Functions.StaticClosure:

static fn (mixed $value): bool => strlen((string) $value) > 0

It's starting to get rather convoluted, and we need to reuse this in a number of different places. This is why I went with the helper function.

I guess we do need to allow mixed and do the casting, and find a better spot for it. Perhaps the helper function could do the array_filter part as well.

mstrelan → created an issue.

I went with the helper function in Unicode, then realised we need the reverse of that so I implemented that too. Since we're only checking if the string is empty it doesn't matter if we were using strlen or mb_strlen before, they would either return 0 or something other than 0 and that's all that matters.

Indeed, the $cid was reset to NULL and the check for $this->cid === '' no longer evaluated to TRUE.

mstrelan → created an issue.

mstrelan → created an issue.

mstrelan → created an issue.

This might be a little contentious since now the "masquerade as any user" permission includes user 1. But since core is moving away from user 1 having special meaning I think this is a good thing.

mstrelan → created an issue.

mstrelan → created an issue.

Made an MR from #3. Hiding patches.

mstrelan → made their first commit to this issue’s fork.

By default Drupal will check the all the route values. Since we checking credentials from the request header all the URL's should be verified.

That's the bug we are trying to fix though. According to the docs for basic_auth → the access should only be applied to routes with the basic_auth option:

  options:
    _auth: [ 'basic_auth' ]

We need to consider if basic auth should apply to the route.

All the child issues are complete. Marking NR rather than Fixed as there are release notes and a change record to be finalised.

These are from the global PHP class
https://www.php.net/manual/en/language.attributes.classes.php

Due to covariance you can add a return type to a child class when the parent or interface doesn't have one. See https://www.php.net/manual/en/language.oop5.variance.php

Postponing on the core issue since it could be close

HEAD was broken. Have rebased and it's green again.

mstrelan → created an issue.

• Remove the duplicate ::userPassResetUrl method and updated the existing user_pass_reset_url function to use current time as discussed with @catch in Slack.
• Updated UserLoginTest::testLoginCacheTagsAndDestination to explicitly submit the login form instead of relying on ::drupalLogin
• Added 📌 Placeholder follow-up for 3469309 Postponed

mstrelan → created an issue.

mstrelan → made their first commit to this issue’s fork.

Added failing test to demonstrate

mstrelan → created an issue.

cspell was due to catch and I debugging some pipeline cache issues, let's move that to 🐛 Hardcode the core project ID for fetching artifacts from the gitlab API Needs review . Have reverted and rebased.

There is a syntax error in the Nonce example, I think csp_nonce and placeholders should be children of #attached.

I reopened 🐛 Background colour of UI widgets get overridden on Ajax load. Active because there is another underlying issue that this hasn't solved. We don't get the css loaded multiple times, but we do get inconsistent order of css files loaded, and that results in the same issue with missing button backgrounds.

Tested on 11.x and 10.4.x and can reproduce there also.

Added steps to reproduce and updated screenshots without contrib.

Re-opening this as it's still an issue in 10.3. Steps to reproduce are the same but you must have the contextual module uninstalled. I believe it ultimately comes down to the contextual/drupal.contextual-toolbar library which ends up loading claro/claro.jquery.ui but the stylesheets are in a different order, so jquery.ui/theme.css takes precedence over claro's button.css.

This issue is not about adding the return types to existing methods, it's just about preventing new methods from being added that don't have a return type. I think adding return types to methods in run time code needs BC consideration most of the time.

Rebased, re-baselined and rewrote the issue summary. FWIW I don't know why some lines were removed from the baseline, but that can only be a good thing as long as the phpstan job still passes.

As per this slack thread it seems the basic_auth module intercepts requests that do not have the basic_auth option set on the route as described in the docs → . If (invalid) auth is provided it will return a 403 even though the route should not be protected.

There are thousands of functions covered by the existing sibling issues, particularly the void one would knock out the most in one go. Then it looks like there are only a few hundred remaining.

I appreciate that the MR is massive but really the only change that needs review at all is to phpstan.neon.dist. Everything else is proven by the phpstan job passing. I don't think committing in chunks will help, this probably just needs "Needs ____ review" but I don't know what ____ is, maybe FM?

drunken monkey → credited mstrelan → .

mstrelan → created an issue.

mstrelan → created an issue.

Down to the last issue 📌 Enforce strict types in tests Fixed . Added a CR.

All child issues are done, I think we can close this unless there is anything I missed.

So from what I can tell some methods are being completely removed like testInvalidLinkExistsExact.

That is one of the tests being removed from WebAssertTest, not a method removed from WebAssert. They're removed because they no longer make sense since the methods are refactored.

That said I think we need to figure out what should be done here and what should be done in #2805849: WebAssert does not track assertions, Test marked as risky → .

$ grep "Method" 8763.diff | wc -l
10753
$ grep "Function" 8763.diff | wc -l
7335

Not sure there's any difference reviewing 10000 or 17000 changes.

Possibly core should do this, but I'm not sure if core has the same problem. I guess we could check if the property exists.

mstrelan → created an issue.

Rebased for 8.x-1.4, only conflict was the use statements in contact_storage.install

mstrelan → made their first commit to this issue’s fork.

Well that was fun. There are three tests remaining that use the login form. One is the UserLoginTest, so we need to keep that. The other two seem to be oddities that could probably be fixed, perhaps in a followup.

It's hard to evaluate if this is overall faster than before because there is such variability in the test suite times. I did see one run that was under five minutes. In my local testing I found a saving of around 200ms per login. So that's roughly 5 mins total savings over 1500 logins.

I've spent a fair bit of time with this MR today on both a large project with many tests, as well as in core, where this is now passing. Some findings:

• Instead of overriding ::drupalLogout we can just use EntityStorageInterface::loadUnchanged in ::drupalLogin
• I see why we needed our own ::userPassResetUrl function. Some tests will log users in more than once in one test, and this leads to invalid one-time login links, since the last login time is greater than the request time
• Calling $this->getSession()->visit() causes session related issues for javascript tests, or when destination redirects are involved. We can simply use ::drupalGet() instead

Opened ✨ Use one-time login link instead of user login form in BrowserTestBase tests Needs review to investigate this in core.

Locally I have \Drupal\Tests\block_content\Functional\BlockContentListTest passing but it looks like there are lots of fails in other tests.

mstrelan → created an issue.

#3303206: Define a standard for adding declare(strict_types=1) →

Where does it say to use a space? There was an issue and standards committee process for this and we settled on no space.

mstrelan → created an issue.

mstrelan → created an issue.

mstrelan → created an issue.

mstrelan → created an issue.

mstrelan → created an issue.

Re-titling as this appears to be addressing properties rather than params.

mstrelan → created an issue.

I'm getting similar failures in 📌 Convert IpAddressBlockingTest to a Unit and Kernel test and improve Fixed ... I think HEAD is broken

mstrelan → created an issue.

I guess this could cause risky tests in contrib, so might need a CR?

Should this have a change record or release note snippet?

Opened #3468236: Adopt phpstan phpdoc types as canonical reference of allowed types → for follow up. Left some responses on the MR. Personally I feel it should go in as-is and anything else should be a follow-up.

mstrelan → created an issue.

nod_ → credited mstrelan → .

mstrelan → created an issue.

Giving up on that MR, since configuring jest and yarn etc is not my strong suit. But this commit shows what the same test would look like as a jest test.

This is getting a bit off-topic though. Let's focus on the more common usage of Nightwatch.