🇦🇺Australia @mstrelan

mstrelan → created an issue.

Moved the constants here. Had to make them public because they are access from UpdateScriptTest.

There are no usages of them in contrib projects (outside of core/modules/system):

https://git.drupalcode.org/search?group_id=2&scope=blobs&search=DRUPAL_C...

https://git.drupalcode.org/search?group_id=2&scope=blobs&search=DRUPAL_C...

I think base classes are similar to interfaces, so see 📌 Enable existing interfaces to add return type hints with a deprecation message for implementors Active

For completeness, if anyone is wondering what happens to modules that may have already updated to the new method signature. The answer is nothing, there is no harm returning void from a subclass if the parent has no return type. https://3v4l.org/B8MS6

Left a comment, not changing status because it's a nit.

This issue rings a bell with an earlier issue I can't recall off the top of my head, but from memory we were doing extra work that shouldn't have been necessary, but was because of inconsistent locations. Left a comment on the MR asking about the sed calls.

mstrelan → created an issue.

I now have maintainer access to the ban project → in contrib. Is now the right time to create the subtree split?

FWIW I came across similar issues in this commit, but ended up not using it. Might be worth a look. I feel like it might be a bit risky extending the phpunit Assert class.

We might need to split this up further, this is what I've identified.

PHP:

• \Drupal\comment\CommentLinkBuilder::buildCommentedEntityLinks
• \Drupal\comment\CommentManager::getCountNewComments
• \Drupal\comment\CommentManagerInterface::getCountNewComments
• \Drupal\comment\CommentViewBuilder::buildComponents
• \Drupal\comment\Hook\CommentHooks::nodeViewAlter
• \Drupal\comment\Hook\CommentViewsHooks::viewsDataAlter
• \Drupal\comment\Plugin\views\field\NodeNewComments::preRender
• \Drupal\Tests\comment\Functional\CommentCSSTest::testCommentClasses
• \Drupal\Tests\comment\Functional\CommentEntityTest::testEntityChanges
• \Drupal\Tests\comment\Functional\CommentNewIndicatorTest::testCommentNewCommentsIndicator
• \Drupal\Tests\comment\Functional\CommentTestBase::$modules
• \Drupal\Tests\comment\Functional\Views\NodeCommentsTest::$modules
• \Drupal\Tests\comment\Unit\CommentLinkBuilderTest::testCommentLinkBuilder

Libraries:

• drupal.comment-new-indicator
• drupal.node-new-comments-link

Views:

• test_new_comments

The whole section is in a module exists check for history, there's an existing issue to move this out

That's a great point, perhaps we can let history.module handle this in contrib, e.g. by providing a hook.

I'd consider making this a duplicate of that issue.

Instead I've postponed it so it doesn't get lost, and moved it to history.module component. I think the sequence should go something like this:

1. Move the existing code to history.module
2. Deprecate history.module in core
3. Move issues for history.module over to contrib
4. Create a hook or other path forward
5. Deprecate relying on the magic named functions

Applied the suggestion from the MR and updated the CR to make it specific to Views UI and with a more realistic example.

📌 Allow to create project for ban module in contrib Active

mstrelan → created an issue.

Blocker is in

This is tricky because neither system nor olivero have a wrapping element, so there is nowhere to apply the attributes. Just because they don't use the variable, it doesn't mean that other templates can't, e.g. starterkit_theme, claro and umami. Not sure what to suggest for a proposed resolution, adding a wrapper in the system theme could mess up styles in sites that don't have an overridden template and aren't expecting div soup to be added. Do you have any suggestions?

Agree we don't normally need a test for a deprecation, unless it's a weird implementation. I think what I really meant that we don't actually have any evidence that the behaviour we're deprecating even works in the first place, so we could test for that as a deprecation test. But I don't know if it's worth it.

Think this is good to go. Failing test is unrelated.

Think this is back to NW for @berdir's latest comment about the ProceduralHookScanStop

I don't think that issue is related

Did the deprecation, needs a change record. Could possibly do a deprecation test but it would involve adding a test module with a form that calls $view->getStandardButtons similar to \Drupal\views_ui\Form\Ajax\ReorderDisplays and also declare a function with the magic naming convention.

Create follow up for CommentManager -> Needs a way to define the class and method.

📌 Deprecate magic ENTITY_TYPE_last_viewed functions Active

Create follow up for ViewsUI -> Needs a way to define the class and method.

📌 Deprecate magic cancel submit handlers in views ui Active

CssOptimizerUnitTest No need to address afaict

We can remove that whole extra namespace at the bottom of the test, since #3034072: Move file uri/scheme functions from file.inc and FileSystem to StreamWrapperManager → . Do you want another follow up or should it happen here?

EntityResolverManager does this just need deprecation? what situation can that be a function?

I think the failing test explains this one. I think it can stay, or update to is_callable.

mstrelan → created an issue.

Opened 📌 Deprecate magic cancel submit handlers in views ui Active

mstrelan → created an issue.

Pretty sure this was fixed in 🐛 A profile without a version causes a fatal error on status report page Active , can anyone confirm? If that's the case a core maintainer can assigned credit and we can close as a duplicate.

Confirmed all hooks still working:

• CommentThemeHooks::preprocessField invoked in \Drupal\Tests\comment\Functional\CommentNonNodeTest
• MediaLibraryThemeHooks::preprocessViewsViewMediaLibrary is invoked in MediaLibraryTestBase::assertMediaLibraryGrid
• ViewsThemeHooks::preprocessNode is invoked in \Drupal\Tests\views\Kernel\Entity\RowEntityRenderersTest::testEntityRenderers
• ViewsThemeHooks::preprocessComment is invoked in \Drupal\Tests\comment\Functional\CommentRssTest::testCommentRss

Postponed on 📌 Convert final 4 preprocess hooks in core modules Active .

Deprecated the properties and added a CR. Left a few review comments of my own, mainly wondering if we need to deprecate the constructor params for the constraint validator?

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

acbramley → credited mstrelan → .

One comment on the @todo item for #939462: Specific preprocess functions for theme hook suggestions are not invoked → otherwise looks good.

Should this be postponed on 📌 [PP-3] Figure out what to do with the install/uninstall modules page Postponed ? Seems we should "figure out what to do" before doing?

Setting to NW because we have a patch that we can start with, but please change to Postponed if we should not continue yet.

I think you misunderstood what @quietone meant. There is no bugfix on the 11.x branch, but if there is a bug it needs to be assessed against the 11.x branch and then backported.

the function `hasAccessToChildMenuItems` in /web/core/modules/system/src/Access/SystemAdminMenuBlockAccessCheck.php

seems to be returning AccessResult::neutral() for some reason.

It's because this loop is empty:

foreach ($this->menuLinkTree->load(NULL, $parameters) as $element)

You need at least one child menu item for this to show.

Moved them down to protected properties so we can see what that looks like.

This seems like it should be the answer but it actually doesn't make sense. They are never accessed like properties. They shouldn't be annotated as @property nor should they be defined as a property, because at the times they are accessed we only have an array.

For example, see \Drupal\Tests\Core\Render\Element\RenderElementTest::testPreRenderAjaxForm. That passes an array to \Drupal\Core\Render\Element\RenderElementBase::preRenderAjaxForm, and in that function we look at $element['#has_garbage_value']. Putting a property on the class doesn't do anything because we don't have an object, we have an array.

This just demonstrates that we're abusing @property annotations to document the Array PI keys. This is consistent with the doc comment right above it:

 * Here is the list of the properties used during the rendering of all render
 * elements. These are available as properties on the render element (handled
 * by magic setter/getter) and also the render array starting with a #
 * character. For example $element['#access'] or $elementObject->access.

For the record, yes I understand that these keys can be accessed as properties using the magic getter, but similarly you could access $obj->foo and $obj->bar, and they are not documented. Unfortunately I don't have a better suggestion for where to document them.

PMNMI for #8. I can't find any evidence that SchemaCheckTrait every mentioned system.mail, which is really confusing because it's clearly in 4b7efe06.

Possibly related:
📌 Tighten config validation schema of system.mail mailer_dsn Fixed
✨ Use structured DSN instead of URI in system.mail mailer_dsn Fixed

The issue summary mentions twice that the current approach is inefficient, but that's not the case. As per #22 ::getTheme already calls ::listInfo, so this is only improving clarity.

Also, instead of try/catch why don't we use \Drupal::service('theme_handler')->themeExists() and return early if that is false.

Thanks for addressing that feedback. I've only just actually tested it now rather than just reading the code and it works well. One thing that stands out to me that could be addressed in a follow up is whether it makes sense to export the created timestamp. It would be a bit weird to have a node or user that was created before a site was created.

I had some additional questions and suggestions for improving the docs, otherwise this is looking really good.

Had a brief look. The test waits for a menu link to be visible, then checks if a grandparent element has a class. The class menu-item--active-trail is added by toolbar.menu.js, so it's feasible that the element is visible before the class has been added. I'd probably try updating the test to use \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForHelper to test the link is visible AND the grandparent has the class.

smustgrave → credited mstrelan → .

I'm not able to reproduce this, is anyone else able to? I note drupal_verify_install_file has not changed much since 2006 other than code style and #2950851: invalid conf file warnings when skip_permissions_hardening is on → .

@heddn that's a great point. I note that TranslationInterface has only one implementation (outside of tests) and that is the TranslationManager. So by using TranslationInterface you'll always be getting TranslationManager, unless another module provides another implementation. The other thing to look at is what methods are actually called. I can see only ::translate and ::formatPlural, both of which come from TranslationInterface, so we could happily receive an object that implements only TranslationInterface and not TranslatorInterface.

I haven't considered BC concerns though, I think it would be mostly fine because we're actually broadening the type in the base class. Even if subclasses are calling methods from TranslatorInterface, it will still work, but phpstan might complain.

smustgrave → credited mstrelan → .

Changes look good to me.

FYI @libbna when there are suggestions on an MR you can just click "apply suggestion" and it will create a commit for you. You can also batch them up and apply them in a single commit too.

mstrelan → created an issue.

Coming here from 🐛 Incorrect type hinting in the smart_trim_tokens function Active . I think we should be able to use string as a type hint, but the preg_match_all in \Drupal\Core\Utility\Token::scan finds ints and floats too. Should core cast these to string?

Related core issue. Probably string is correct but the regex is finding ints.

I think it's possible we might be trying to access ->uuid() on null in the link event subscriber. Other than that most of the other comments I made are nits.

mstrelan → created an issue.

mstrelan → created an issue.

Sorry if I've missed something but why are we deprecating functions that never existed before? Any how come the originals have been renamed? I've definitely seen a template_preprocess function called directly before, even if that seems like a bad idea.

Rebased against 11.x but now the MR needs to be updated to point to that branch, which I can't do.

The logic is a bit confusing though with the multiple conditions and overrides, I'm sure it could be refactored to be easier to read.

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

I guess since Hooks classes are now autoloaded unconditionally it doesn't make sense to check if a method exists. I've updated the test to check if the hook implementation exists instead.

The assertion was added in #2160091-61: drupal_rebuild() rebuilds container twice, since drupal_flush_all_caches() also rebuilds it → (specifically comment #61). Have not reviewed closely, just identifying where it happened.

Re polyfill:
Regardless of core version no reason XB couldn't add it as a dep, just give it a loose constraint so it doesn't conflict with core.

Yes this should detect subdirectories

Added complete steps to move all the identified modules if we decide to proceed with this.

Pushed up a proof of concept, needs review for the idea.

mstrelan → created an issue.

OK that test fail suggests this might not be a novice issue. IIRC there is another issue for ignoring hooks from modules that are not installed. This might be a duplicate of that.

mstrelan → created an issue.

Some other classes that previously extended CacheTestBase were replaced in #1637478: Add a PHP array cache backend → , not sure why ClearTest wasn't replaced at the same time. We also have \Drupal\KernelTests\Core\Cache\GenericCacheBackendUnitTestBase::testDeleteAll but that doesn't necessarily cover drupal_flush_all_caches.

DrupalFlushAllCachesTest has the @covers ::drupal_flush_all_caches annotation, so that's a good start. But it seems to only be testing that container.modules is updated.

Not sure we need any of this coverage, but we could maybe set a couple of cache items then check they're not there afterrwards in DrupalFlushAllCachesTest?

Yeah we could just move ClearTest::testFlushAllCaches in to DrupalFlushAllCachesTest. Probably that class should test everything that function does, like deleting asset files, twig caches etc. I guess those are all tested independently so it might be hard to find something useful to test.

Unrelated to this issue, but this line in DrupalFlushAllCachesTest is useless now since that function has moved to OOP hooks.
$this->assertFalse(function_exists('system_test_help'));

I don't think we need to do the test blocks. Category is still optional, we're just ensuring core blocks set the category. I've updated the example in block.api.php

I'm still unsure about the fallback translation in CategorizingPluginManagerTrait. If we have that, why bother enforcing all the core blocks have a category?

Should we move this to coder? There is at least one historic issue I know of where coder is parsing info.yml files - ✨ Ensure module dependencies are an array in .info.yml Fixed

@quietone sure, if no one else wants to in the short term then I can. I tried to created the project but the short name "ban" is reserved. Any tips?

Actually let's close this as a duplicate of 📌 Add validation constraints to system.site Active . This existed first, but the other issue is far more modern.

I've rerolled this to follow similar logic to the advisories check. We will need to add some caching because this is processed on every admin page and \Drupal\system\SystemManager::checkRequirements loads a lot of files and invokes a lot of hooks.

This would also be quite annoying in local environments that are either on 11.x or have the rebuild_access setting enabled. This is what it looks like for my core env:

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

Hiding patches as we have an MR. Added steps to reproduce. Updated title to better align with usability review.

The system_retrieve_file() function was deprecated in 📌 deprecate system_retrieve_file() without replacement Fixed and removed in 📌 [11.x] Remove deprecated code from system module Needs review .

Updated the MR so choosing the Text Embedding (Vector) data type creates the field as a text field. Then the field mapper is decorated to create a paired knn_vector field. That will let us dynamically create the ingest pipeline.

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

For many of these we should probably autoconfigure the loggers instead Loggers can be autoconfigured for service classes implementing \Psr\Log\LoggerAwareInterface →

Is this still an issue? I was unable to reproduce the issue after installing 11.x with minimal profile. I put a breakpoint in \Drupal\system\Install\Requirements\SystemRequirements::checkRequirements after $info = $module_extension_list->getExtensionInfo($profile); and can see the profile info has been loaded.

Removing the tag since usability review was given in #59. Not really sure the next steps here. Adding a related issue.

This seems very similar to 🐛 Invalidate rendered cache tag if config.system.site translate Postponed: needs info which I wasn't able to reproduce. Tried to reproduce this as well and couldn't. Can we please get steps to reproduce? Here's what I tried.

1. Install Drupal 11.x with standard profile, includes dynamic page cache and page cache modules
2. Ensure render cache is not disabled in settings.php
3. Visit the home page as an anonymous user, observe site name in the branding block
4. drush config-set system.site name "Test 1"
5. Repeat step 3

Rebased the MR and added screenshots and user interface changes to the issue summary. I'm not sure the change record is really necessary.

The one minor concern I have is that it's now unclear if you were to move your Drupal installation to another location, would the file system paths remain in their original locations? That would depend on what's in your settings.php file, and you can no longer determine the answer from the config page.

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

Adding the word "config" to the title for clarity. Changing the component also. The report might end up living in system.module, but I think the concept and most of the logic need to be part of the config system.

There are 81 instances of CacheTestBase, but most of them look like clones of Drupal core.

Excluding the path core/modules/system there is 1 instance and that is not a code reference but some kind of git log reference. Going to self-restore to RTBC based on this.

Let's catch \Drupal\Core\Extension\Exception\UnknownExtensionException instead

If that's the case then change step 3 from #3 to "Check the checkbox for Navigation module".

That results in the following status message:

You must install the Layout Builder, Layout Discovery modules to install Navigation.

Can we confirm that's what this is about?

Added a possibly related issue.

Checked starterkit_theme and it already has this class too. Created an MR, hiding patches.

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