🇦🇺Australia @mstrelan

Rebased the MR.

The first thread appears to have an open discussion.

I have refactored this as requested.

For the workspace one should we postpone till workspace is refactored?

Don't think so, there is no harm in leaving it in.

I have reviewed MR 10612 and appreciate the updated issue summary. Had a question on the MR but I think it's a non-issue. All looks good to me and pipeline is green.

Answered the question in the MR and rebased

Rerolled

Rerolled

As per slack it's probably not necessary to skip caching when $libraries_to_load is empty, as there is still a fair bit of logic that can be cached, such as hook invocations and asset optimisation.

This looks great. I stepped through this with the problematic webform mentioned in the IS. At first I thought it was still problematic in that it was still getting and setting the cache when $libraries_to_load was empty, but then I was relieved to see that $asset_settings was still merged in regardless of what happened with the cache.

mstrelan → created an issue.

First of all I was going to say we should hash the settings into the cache key, but the settings logic isn't cached.

I don't think we can hash the settings, they contain ajaxTrustedUrl which has keys like form_action_p_pvdeGsVG5zNF_XLGPTvYSKCf43t8qZYSwcfZl2uzM and ajax which has keys like edit-actions-submit--mclJzp_PNkM. It seems at least the edit-actions-submit suffix changes on each request.

Some of the settings logic is cached, and the rest of it is not. I will admit I don't really understand what's going on in this function.

I've pushed a second branch which attempts to do zero libraries work if there are no libraries, but also doesn't cache anything. Didn't run tests on that yet, approach could be completely flawed.

Seem to be a few test fails related to ajax, not sure if they are random fails and I don't see a retry button.

The whole function could use a refactor, but that's probably blocked on #2614936: Improve unit test coverage for AssetResolver → .

mstrelan → changed the visibility of the branch 3494385-gitlab-fail to hidden.

mstrelan → changed the visibility of the branch 3494385-block-user to hidden.

mstrelan → created an issue.

Needs tests etc but setting NR to get eyes on it

mstrelan → created an issue.

mstrelan → created an issue.

This has been included in the 3.0.0-alpha4 → release.

@geek-merlin any chance you'd be able to put up an MR with that approach, even if it's only a WIP / POC?

larowlan → credited mstrelan → .

Rebased and regenerated the baseline for 4c45e111

Wow, blast from the past here.

With IE gone also.

Not only is IE gone, this was only an issue in IE7! Furthermore, this issue pre-dated Drupal's use of Git, as the patch applied to CVS.

Is this about hook_process or hook_process_HOOK? I think the title needs to be updated.

In light of #24 I think we should revert the changes to the Inspector class and simply add the polyfill without any usage. Alternatively we could find somewhere else to use it.

mstrelan → created an issue.

@chi see #9 and the issue linked there for discussion on casing

dww → credited mstrelan → .

There are a number of references to authorize.php that need updating

$ grep -r "authorize.php" core
core/lib/Drupal/Core/DrupalKernel.php:      // authorize.php, and others to auto-detect a base path.
core/lib/Drupal/Core/Render/BareHtmlPageRendererInterface.php: * - authorize.php
core/lib/Drupal/Core/File/file.api.php: * manager when they are redirected to the authorize.php script to authorize
core/lib/Drupal/Core/File/file.api.php: *     the authorize.php form.
core/lib/Drupal/Core/File/file.api.php: * @see authorize.php
core/themes/stable9/templates/admin/authorize-report.html.twig: * Theme override for authorize.php operation report templates.
core/themes/stable9/templates/admin/authorize-report.html.twig: * This report displays the results of an operation run via authorize.php.
core/assets/scaffold/files/htaccess:  # Allow access to PHP files in /core (like authorize.php or install.php):

mstrelan → created an issue.

Checked again and there are only 20, not 44, ever since 📌 Add return types to update_test_* hooks Active . Have updated the IS to reflect that.

MR looks good, pipeline is green, 120 lines removed from baseline checks out (20 x 6 = 120). RTBC from me.

Needs work for the config form

mstrelan → created an issue.

Added a couple comments on the MR

Re #9: there are plenty of hooks that could be refactored one way or another, but that's not really in scope here, so I'd prefer to leave that out.

mstrelan → created an issue.

mstrelan → created an issue.

mstrelan → created an issue.

@tstoeckler that's great. I realised we could probably tackle all hook_update_N implementations in one hit. Opened 📌 Add return types to hook_update_N implementations Active if anyone wants to work on it.

mstrelan → created an issue.

Need to regenerate the baseline

mstrelan → created an issue.

mstrelan → created an issue.

Rebased with these steps:

• git rebase -i 11.x
• Drop the baseline commit
• Regenerate baseline with ./vendor/bin/phpstan --configuration=core/phpstan.neon.dist --generate-baseline=core/.phpstan-baseline.php
• git add core/.phpstan-baseline.php
• Commit and force push

Best not to manually resolve conflicts in the baseline, just regenerate it.

Rebased

Added steps to reproduce. Note the grep pattern is excluding jsonapi because these implement hook_jsonapi_ENTITY_TYPE_filter_access and return array.

Needs rebase

Needs rebase

Rebased and added return type to methods listed in #8.

Also added AccessResult::neutral to MediaLibraryHooks::imageStyleAccess when nothing else is returned.

📌 Bump PHPStan to version 2.0.0 Active

Ran in to the same issue with the view revision operation.

I'm not sure about that, it seems to be failing in the same way for previous major which is on Drupal 10.3.9.

A few more:

\Drupal\image_module_test\Hook\ImageModuleTestHooks::imageStylePresave
\Drupal\layout_builder\Hook\LayoutBuilderHooks::fieldConfigDelete
\Drupal\layout_builder\Hook\LayoutBuilderHooks::fieldConfigInsert
\Drupal\menu_link_content\Hook\MenuLinkContentHooks::menuDelete

Found a few more for hook_ENTITY_TYPE_access and hook_ENTITY_TYPE_create_access:

Drupal\field_ui_test\Hook\FieldUiTestHooks::fieldConfigAccess()
Drupal\layout_builder\Hook\LayoutBuilderHooks::blockContentAccess()
Drupal\media_library\Hook\MediaLibraryHooks::imageStyleAccess()
Drupal\media_library_test\Hook\MediaLibraryTestHooks::mediaCreateAccess()
Drupal\node\Hook\NodeHooks1::nodeAccess()
Drupal\node_access_test\Hook\NodeAccessTestHooks::nodeAccess()
Drupal\config_test_rest\Hook\ConfigTestRestHooks::configTestAccess()
Drupal\user_access_test\Hook\UserAccessTestHooks::userAccess()
Drupal\workflow_type_test\Hook\WorkflowTypeTestHooks::workflowAccess()
Drupal\workspace_access_test\Hook\WorkspaceAccessTestHooks::workspaceAccess()

mstrelan → created an issue.

Added the param

Addressed most feedback, 2 outstanding items in the MR for re-review.

Responded to some of the MR feedback, need further advice. Will look at the entity.api.php feedback too, it wasn't loading in the MR though.

Re #5:

• The *_hook_infohooks were missed because my script was looking for a #[Hook] attribute in Hook classes. Could possibly be skipped due to 📌 Deprecate hook_hook_info() Active
• test_module_hook_info is actually in a heredoc string for writing a .module in a test, but I guess it could be included too
• HookCollectorPass is just a comment talking about info hooks

mstrelan → created an issue.

Failing tests are unrelated

mstrelan → created an issue.

I'd be in favour of eventually dropping "implements" comments, but it does have some use at the moment, particularly with these ENTITY_TYPE replacements. For example, see ContentModerationHooks::workflowInsert. It's not clear from #[Hook('workflow_insert')] that workflow is an entity type. Would need something like #[EntityTypeHook(hook: 'insert', entity_type: 'workflow')] or #[Hook('ENTITY_TYPE_insert', replacements: ['ENTITY_TYPE' => 'workflow'])]

Added some hook_ENTITY_TYPE_* hooks that had the entity type in place of ENTITY_TYPE. Note in many cases where ENTITY_TYPE is used correctly it specifies the entity type afterwards, e.g.

Implements hook_ENTITY_TYPE_create() for 'config_test'.

We should probably do the same.

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.