🇦🇺Australia @mstrelan

I think hook_entity_bundle_create is meant to be in the entity_crud group too, but the doc has @see instead of @ingroup, which we should fix too. That would give us the following extras:

• Drupal\field_ui\Hook\FieldUiHooks::entityBundleCreate()
• Drupal\jsonapi\Hook\JsonapiHooks::entityBundleCreate()
• Drupal\entity_schema_test\Hook\EntitySchemaTestHooks::entityBundleCreate()

And some more for the list in #4:

• \Drupal\field_ui\Hook\FieldUiHooks::entityViewModePresave
• \Drupal\field_ui\Hook\FieldUiHooks::entityFormModePresave
• \Drupal\field_ui\Hook\FieldUiHooks::entityViewModeDelete
• \Drupal\field_ui\Hook\FieldUiHooks::entityFormModeDelete

Missed a few that don't have the right docs, i.e. "Implements hook_ENTITY_TYPE_":

• Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentInsert()
• Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentPresave()
• Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentUpdate()
• Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentView()
• Drupal\config_test\Hook\ConfigTestHooksHooks::configTestDelete()
• Drupal\config_test\Hook\ConfigTestHooksHooks::configTestInsert()
• Drupal\config_test\Hook\ConfigTestHooksHooks::configTestLoad()
• Drupal\config_test\Hook\ConfigTestHooksHooks::configTestPredelete()
• Drupal\config_test\Hook\ConfigTestHooksHooks::configTestPresave()
• Drupal\config_test\Hook\ConfigTestHooksHooks::configTestUpdate()

mstrelan → created an issue.

mstrelan → created an issue.

Updated ContentModerationHooks::entityAccess to return AccessResult::neutral() instead of NULL.

All good, it's been going ok so far and rebasing these is easy as I just drop the baseline commit and regenerate it. I also receive the "MR can no longer be merged due to conflict" emails so been trying to keep them up to date so they're mergeable when reviewers or committers get to them.

There is at least one instance in ContentModerationHooks where this might return null instead of an AccessResultInterface. It should probably return a neutral access result.

mstrelan → created an issue.

Rebased

Some statistics so far.

We started with 1907 procedural functions with no return type specified. Hooks are OOP now, so they match a different grep pattern.

If we use the original grep pattern we can see how many non-hook functions are left in the baseline, which are out of scope for this meta:

$ grep "Function .* has no return type specified" core/.phpstan-baseline.php | wc -l
511

That means we had roughly 1396 hook implementations to update.

If we use this new grep pattern we can see how many hook implementations with no return type are remaining:

$ grep "Method Drupal\\\\.*\\\\Hook\\\\.* has no return type specified" core/.phpstan-baseline.php | wc -l
834

After 📌 Add void return type to all *_alter hook implementations Active this drops to 587.

I believe this means we could remove bleeding edge from phpstan config. To be discussed if that's something we want to do though.

Rebased once more

Given there are already 12000+ sites using 3.x I'd suggest releasing a stable version of what's in the alpha that supports 11.x, and open up a 4.x branch to investigate the package mentioned in #4.

This has exposed a few alter hooks that are erroneously returning something.

mstrelan → created an issue.

This was made worse by the conversion to OOP hooks. It seems that any hook implementation that had a return type before now has a space before the colon.

mstrelan → created an issue.

For anyone wanting to pick this up, here is a command to add the return type. Will need to redo all the other fixes for functions that don't return anything though.

find . -type f -path "*/src/Hook/*.php" -exec sed -i "/#\[Hook('help')\]/ {N;s/\(public function \w*([^)]*)\)/\1: void/}" {} +

Updated title with identifier and added steps to reproduce. There are currently 14 remaining errors.

Is this a dupe of the existing 🌱 [meta] Fix 'should return {type} but return statement is missing' PHPStan L0 errors Active or do we get more results with the strict package?

Updating IS to reflect changes to PHPStan over the last couple years (baseline, identifiers)

Added one small docs thing to fix

I think this fix is just covering up a code smell. The method getExpectedUnauthorizedEntityAccessCacheability exists in \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase but not in \Drupal\Tests\rest\Functional\ResourceTestBase, yet the test trait that calls this method is added to classes that don't extend from EntityResourceTestBase. If we're saying this method needs to exist in all child classes then we might as well just add it to the base, but that makes it clear that it doesn't belong there. Ideally it should be refactored out of the shared trait, or failing that we should check if $this instanceof EntityResourceTestBase before calling that function.

@nicxvan yes, was rebased first, and I checked it merges cleanly to 11.x

I added checkFunctionNameCase: true to the existing phpstan config and got the same result. As per #3487218-7: Resolve PHPStan-Strict errors: class.nameCase → we should fix these with or without phpstan enforcement, as they are clearly incorrect.

Have updated the IS with steps to reproduce not requiring the strict package.

I have to agree with @dpi's summary in #3487269-9: [meta] Resolve issues exposed by PHPStan Strict with all rules off (misc and core configuration changes) → , these can and should be fixed regardless if phpstan or phpstan-strict are tools we use or don't use.

I've reviewed the MR and noted all changes are straightforward, and correctly match the expected class names. I also ran phpstan with checkInternalClassCaseSensitivity: true (without the strict package) and verified the same 8 violations were reported.

Have updated the IS with steps to reproduce not requiring the strict package.

mstrelan → created an issue.

Rebased once more

Thanks @tstoeckler, this looks good to me. Will leave for someone else to RTBC since I did the original MR.

You don't need a new major to drop D9 support. But if you decide to do that anyway you might like to take the chance to move to semver and cut 2.0.0

If you want D9 users to be happy ideally you should release 8.x-1.6 that rolls back the breaking change and restore it in 8.x-1.7 with a constraint on D10+. That will give the smoothest update path for all users.

smustgrave → credited mstrelan → .

I suspect the fails in the pipeline are random fails, but I'm not seeing the button to retry.

EntityFormDisplay::processForm seems to be the culprit. Setting to Needs Review for feedback.

Here's a start. Needs work for implementing in plugins.

mstrelan → created an issue.

This is working for the fields in the _field_selector container. For the usual field widgets, it is setting weights correctly, but these are not being rendered in the right order. I'm unsure why that is.

Re-rolled

amateescu → credited mstrelan → .

Thanks, but these were known random failures, unrelated to this issue. I've retried the pipeline and it passed.

Needs a re-roll

Added a failing test

mstrelan → created an issue.

Is this a duplicate of 🐛 Roles can be deleted when upgrading from 9.5 to 10 if a module had removed permissions Active ?

mstrelan → created an issue.

I tested this on 11.0.x and had a few issues.

1. \Drupal\image\Controller\ImageStyleDownloadController::deliver now expects 4 params but we're only passing 3, for testing I hardcoded 'public' as the fourth param
2. image/avif was not listed in $request->getAcceptableContentTypes(), I'm not sure yet why not, but I commented out this check to test if everything else was working
3. I had issues getting the conversion to work with ImageMagick. Not sure why, but after switching to the GD patches I was able to get this to work

Ok that all makes sense about refactoring and keeping them separate.

The browser test drupalGet() doesn't return anything, so why should the kernel test version?

It does return something:

   * @return string
   *   The retrieved HTML string, also available as $this->getRawContent()

See: https://git.drupalcode.org/project/drupal/-/blob/11.0.5/core/tests/Drupa...

#8 You shouldn't need to replace the current baseline, the command will overwrite what you have already.

I'm not quite convinced about return an empty string though. I guess it's fine, but I think returning NULL or FALSE would be better, as per jsonapi_help, but we'd then also need to update the api doc to allow that in the @param annotation.

While we're at it, why does every hook_help implementation have a switch statement, just to check a single case? Is there a scenario where hook_help would return something for multiple routes?

I did some further tinkering with this. Wondering if we actually need our own ::drupalGet in the trait rather than just using the one from UiHelperTrait? I tried it out and it worked, but I had to implement ::prepareRequest as an empty function.

I wonder if drupalGet, drupalLogin and drupalLogout should be moved out of UiHelperTrait, they have nothing to do with the UI, whereas the rest of that trait has methods like click, clickLink, etc which make a lot more sense. Then we would rename this trait something like HttpKernelHelperTrait (without the word UI) and it would use whatever trait has drupalGet. Same applies for the assertSession method, we don't need to duplicate it if we can share the existing one.

Not sure if this helps or just complicate things.

quietone → credited mstrelan → .

Rebased and rebaselined

Rebased and rebaselined

mstrelan → created an issue.

@pmagunia probably best to open a separate issue for that. Left feedback regardless.

In practice we have snippets that combine <script>, <style> and <link> tags in the same snippet, so unfortunately the original approach of adding a checkbox doesn't work. Have updated the approach to provide a separate field for entering the javascript, and repurposed the existing field for supporting markup.

This looks great, thanks to everyone who worked on it.

@larowlan do you think we should add a test case for media revisions as well, so all core entity types with the generic revision UI are supported?

@hctom can you provide more info about your Drupal version and any other steps to reproduce it? The test coverage suggests that block content is working so perhaps you have a patch or custom/contrib module that is interfering?

I like this in principle, but worry it will be harder to find the test results summary. Currently you need to go to the child job so see that, now you may need to go to two child jobs. I wonder if there's any way to bubble up the test results to the parent job.

• hook_preprocess_HOOK should be updated here
• hook_template_preprocess_default_variables_alter is a separate hook. I think once we get through the most common hooks we could batch a bunch of less common hooks together.
• _claro_preprocess_file_and_image_widget is not a hook implementation
• _template_preprocess_default_variables is not a hook implementation
• test_theme_preprocess_theme_test_preprocess_suggestions__* look like they should be updated here
• theme_test_theme_suggestions_theme_test_preprocess_suggestions implements theme_test_theme_suggestions_theme_test_preprocess_suggestions and furthermore it returns an array

None of the functions in #5 are form alters, but their names are similar to form alters. For example some of them implement hook_entity_form_display_alter and others have the word "form" in the module name and alter some other hook.

Postponing on 📌 Add array return type to all hook_removed_post_updates implementations Active as it doesn't make sense to rebase each of these, let's do one at a time.

Ah, the phpdoc comment didn't match the right format, so my find command didn't find it. Have rebased, updated, and fixed the docs at the same time.

I did some digging, and while I don't have any answers I do have some further information.

The test that was triggering the error looks like this:

$this->setCurrentUser($this->createUser()->addRole('regional_manager'));
$url = Url::fromRoute('my_module.my_route');
$request = Request::create($url->toString(), 'HEAD');
$response = \Drupal::service('http_kernel')->handle($request);

But if I change this to use ::drupalLogin and ::drupalGet then the bag is decorated correctly.

Similarly, If I remove the access toolbar permission from the role and revert to the original test, then MasqueradeCacheContext::getContext does not get invoked and the test passes.

Finally, if I remove the masquerade_switch_back item from masquerade_toolbar then again the test passes.

Hopefully that helps to explain this.

The diff is + 3346 − 2771, I certainly didn't do that by hand. The pipeline for 7f4b4891 spat out the patch and I applied it. I would guess that is perhaps too disruptive as it will conflict with existing merge requests, but on the other hand it might be better to bite the bullet and get it done.

Good idea @quetone, I had thought about doing the same. I think fixing most of 📌 Fix strict type errors detected by phpstan Active would go a long way to getting this green. From memory there were no reported errors in core/lib/Drupal/Component, so maybe we could try limit to just that area first.

I couldn't get it working merging the bits from the default config so I tried just removing the custom config altogether. Turns out the config that core uses is massively different to what webform uses, so it's resulted in a very large patch. Not sure where to go from here, giving up for now.

The majority of the fails are in yml files, only a handful are in js files. It looks like eslint is treating yaml as js, and digging a little deeper it's probably because webform has its own .eslintrc.json file that's missing some config that the default pipelines has. Going to try merge some of the defaults from https://git.drupalcode.org/project/gitlab_templates/-/blob/main/assets/i... and see what we get.

I'm not sure this is necessary anymore. It was an idea to solve 🐛 Admin Toolbar Tools breaks Dynamic Page Cache on Drupal 10.3 when update module is enabled Active but was addressed in ✨ Conditionally disable access to update manager routes Needs work . There is a chance there is still merit in doing this, but I think we'd need to find a real use case first.

This looks good now

mstrelan → created an issue.

mstrelan → created an issue.

That sounds like a good idea, there is still one void union type that needs updating.

Rebased

Sorry I omitted that. Essentially use \weitzman\DrupalTestTraits\Entity\UserCreationTrait::createUser and \Drupal\Tests\UiHelperTrait::drupalLogin.

Example:

$user = $this->createUser();
$user->addRole('MY_ROLE_ID');
$user->save();
$this->drupalLogin($user);

In a client project I have a trait like this:

<?php

declare(strict_types=1);

namespace Drupal\Tests\my_module;

use GuzzleHttp\Cookie\CookieJar;
use Symfony\Component\BrowserKit\CookieJar as BrowserKitCookieJar;

trait SessionCookiesTestTrait {

  public function getSessionCookies(): CookieJar {
    /** @var \Behat\Mink\Driver\BrowserKitDriver $driver */
    $driver = $this->getSession()->getDriver();
    $cookieJar = $driver->getClient()->getCookieJar();
    $this->assertInstanceOf(BrowserKitCookieJar::class, $cookieJar);
    $domain = \parse_url($this->baseUrl, PHP_URL_HOST);
    \assert(\is_string($domain));
    return CookieJar::fromArray(
      $cookieJar->allRawValues($this->baseUrl),
      $domain,
    );
  }

}

Usage example:

\Drupal::service('http_client')->request('POST', $this->buildUrl($url), [
  'cookies' => $this->getSessionCookies(),
  'form_params' => ['ids' => $ids, 'tokens' => $tokens],
  'http_errors' => FALSE,
]);

Would something like that work for you? Would it make sense to include this in DTT?

It's in the existing docblock for createTableSql

In fact, why do we have methods that just throw exceptions? Maybe they should belong to an interface instead and the expected return type can be documented there. One of the methods has a todo suggesting it could be moved to an abstract method. I think this needs further investigation.

This is a bit weird, because native return types don't allow for void to be used in a union type. It seems acceptable in the @return phpdoc annotation, but when we eventually introduce native return types for these we'll need to return something.

Thanks for the review. I'm not sure there's much point updating these files generated in tests. The point of this issue is to reduce the baseline and maybe one day get to a point where there is a phpcs rule that enforces this, but the contents of this generated file doesn't really conform to any other coding standards. It will never be scanned by phpcs nor phpstan. It also adds to the cognitive load for reviewer or committer as it looks different to the rest of the diff.

Setting back to Needs Review, if you feel strongly set it back again and I'll work on it.

mstrelan → created an issue.

Wrong component

mstrelan → created an issue.

Unfortunately many of these don't return anything if the switch statement doesn't find a matching case. We probably need to update those to return an empty string or perhaps null or false. The empty string would be the simplest, null or false would require a union type signature.

We also need to regenerate the phpstan baseline.

mstrelan → created an issue.