🇦🇺Australia @mstrelan

kim.pepper → credited mstrelan → .

mstrelan → created an issue.

Changing to NW because we have an MR. Updated the IS to expand the scope to all @return docs. It might be possible to detect and automate this, but in the meantime we might as well fix the ones we've already identified. We could open a follow up to automate it.

Tests are mostly passing now by enabling promote module in failing tests. I'm now thinking that in order to make this easier to review we should initially make the promote module a dependency of node. We can then have a follow up to remove that dependency. That would also give us a bit of a safety net if there are edge case integrations that we haven't covered.

Happy with that, will leave to a committer to decide on BC implications.

Between @danrod, @bramdriesen and myself we've got 47 years of experience to get this novice issue in. I tagged it hoping it could help a new contributor. Nevertheless, let's get this in, maybe we can all opt out of receiving credit for this one.

Only one thing I'm not sure about in LinkFieldTest, not changing status here.

Looks great, only 3 minor comments

I guess we would need to determine what to do with the default frontpage view. It likely needs to move to the promote module. Perhaps node may need to ship a version without the promote filter and the promote module can override it. Or perhaps node should depend on promote initially until some of these things are solved.

Opened 📌 Ensure that Ban does not get special core treatment Postponed

mstrelan → created an issue.

Test fail (twice) seems unrelated. This is the code on the status report page:

// Ensure the status is not a warning if APCu size is greater than or equal
// to the recommended size.
if (preg_match('/^Enabled \((.*)\)$/', $elements[0]->getText(), $matches)) {
if (Bytes::toNumber($matches[1]) >= 1024 * 1024 * 32) {
$this->assertFalse($elements[0]->find('xpath', '../../summary')->hasClass('system-status-report__status-icon--warning'));
}
}

And this is the browser output https://issue.pages.drupalcode.org/-/drupal-3538277/-/jobs/6109542/artif...

I think we should expand the scope to change this everywhere _controller is used.

The constant is currently used in these places:

\Drupal\jsonapi\Revisions\ResourceVersionRouteEnhancer::enhance
\Drupal\jsonapi\Routing\Routes
\Drupal\Tests\jsonapi\Unit\Routing\RoutesTest

Should we update in the following places too:

\Drupal\Core\Controller\ControllerResolver
\Drupal\Core\Controller\ControllerResolverInterface (docs)
\Drupal\Core\Entity\Enhancer\EntityRouteEnhancer
\Drupal\Core\Entity\EntityResolverManager
\Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider
\Drupal\Core\Routing\Enhancer\FormRouteEnhancer
... probably some other places too

mstrelan → created an issue.

@tanushree gupta I see a fork has been created but no code has been pushed to it. Please reach out if you are having trouble, or if you're no longer working on it you can leave a comment so another novice contributor may pick it up.

mstrelan → created an issue.

mstrelan → created an issue.

@nicxvan can you explain why, or point to the discussion where that decision was made? I would have thought that issue would be a simple wildcard pattern somewhere and this would not disrupt that.

It seems the $has_data lock was added to the uri_scheme in comment #17 of #367013-17: Field update (field_update_field) → with no discussion as to why. Here's the commit db09a617. The intent of that issue was to prevent changes to fields that would introduce schema changes, but there is no schema change here.

Can we start the new module that will hold the plugin but not deprecate it yet? So we can move any migrations and tokens to it? Or does the module need to be added deprecated?

I would think it cannot be deprecated until there is a replacement, and that replacement would be the contrib module. The contrib module would not exist until the core module exists.

I don't necessarily agree with #31. Most of the time editors are not aware of the difference of public / private. Therefore it should be up to site builders to determine how to communicate this.

We have a use case for converting a file field on a media bundle to use private, to allow for an editorial workflow. The expectation is that files attached to draft and archived media entities should not be accessible to anonymous users. We already have existing files in public, and we can either choose to move them all to private, or leave them as they are and only use private file system for new files. We could even implement a presave hook that moves files to the private file system when the media entity is archived.

I think this is one of those cases where we are better off just getting the bug fixed rather than trying to work out how to test it. Updated proposed resolution.

I just found #514056: Move sticky, promote and user.blocked to flag field types in core. → that was closed as a duplicate of #29338: Hide Promoted/Sticky fields by default in Form display → before that was rescoped to only hiding by default. Potentially it could be reopened now and we close this as a duplicate, but it's maybe cleaner to just continue on here.

Updated the proposed resolution based on #4. Note that the general idea was discussed early on in 29338. Also I guess this is no longer postponed, so setting to needs work, but noting the issue tags.

It's linked at the top of the IS, but it only goes as far as creating the project with the subtree split, doesn't mention the namespacing issue. I can try update it tomorrow.

Made a start as a proof of concept. There is a lot to do, and not sure there is any good way to split it up. Might be best to park it until we can decide if this is even something we want to do.

Added a section to the docs - https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsol... →

Added section on ban.module. Fixed typo for book.module.

Would it be possible to remove the base field definitions from node module and move them to a separate module that implements hook_entity_base_field_info? Then we install that module for existing sites with an update hook. After a full major cycle we move the module to contrib. We would provide good documentation for how to remove the module if you're not using these fields.

There will likely be a lot to untangle:

• Getters and setters in Node class can be deprecated, and we can deprecate magic access in __get/__set
• Node constants can be deprecated and move to the new module
• Actions (system.action.node_make_sticky, etc) can move directly to the module
• Config entities (views, entity view displays, etc) that use these fields and actions will need a dependency on the new module
• Node views data moves to the new module
• Not sure about NodeStorageSchema
• Relevant parts of \Drupal\node\NodeAccessControlHandler::checkFieldAccess can be implemented in hook_entity_field_access[_alter]
• Other code paths will need to move to hook implementations in the new module
• We may need some new hooks

You're right, it was 16 years ago, it was the last updated date on the issue that was 11 years ago.

Follow up 📌 Decide the fate of Drupal\Tests\system\Kernel\Module\VersionTest Active

mstrelan → created an issue.

Added a couple comments on the MR.

FWIW 📌 Remove comment and node preview Active , but that's not likely to get in quickly, so let's continue here.

mstrelan → created an issue.

Any chance of a backport to 2.7?

#8 is done, we can continue here

OK we now have Ban 1.0.0 → .

FWIW I struggled a bit with knowing exactly what steps were needed for the contrib project, because core project names are reserved and there are weird namespace issues to know about. Steps should be updated something like this:

• Create an issue in the infrastructure queue to get the project created - e.g. 📌 Allow to create project for ban module in contrib Active
• Tag a stable release, note that the project name will be duplicated - drupal/PROJECT-PROJECT
• Once the release has been tested and core deprecations are complete, open an issue in the project_composer issue queue to fix the namespace

I think we can move over to 📌 Deprecate the Ban module Active now.

The only place I use preview is on drupal.org, and that's because the text editor does not have any formatting. This won't be an issue anymore once issues live on Gitlab, although not sure about projects.

I'm trying to make sense of the next steps, it seems incredibly wasteful to throw away the hours of work we've already done. Nevertheless, we need to find which subsystem is the smallest diffstat.

core/modules/comment is +61-34
core/modules/link is +60-34
core/modules/node is +45-22

But then there are other files that are touched:

link subsystem:
core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
core/modules/menu_link_content/src/Entity/MenuLinkContent.php
core/modules/shortcut/src/Entity/Shortcut.php
core/tests/Drupal/FunctionalTests/Routing/RouteCachingLanguageTest.php

comment subsystem:
core/modules/search/tests/src/Functional/SearchCommentTest.php

node subsystem:
core/modules/views/tests/src/Kernel/TestViewsTest.php
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php

core/modules/system/system.module - I guess we deprecate after we've made all the other changes

So node is clearly the smallest. I've opened 📌 Introduce and make use of NodePreview enum, to replace node related usage of DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED constants Active to start there.

mstrelan → created an issue.

I tried to search contrib to see if this was widely used still, but ran into problems with the fuzziness (it refusing to do an exact search for _cancel().

Apparently this is a known issue - https://git.drupalcode.org/help/user/search/advanced_search.md#known-issues

The search query must not contain any of the following characters:
. , : ; / ` ' = ? $ & ^ | < > ( ) { } [ ] @

So perhaps http://codcontrib.hank.vps-private.net/search?text=_cancel%28&filename= is a better way to search.

Bumped removed version to 13.

So is this RTBC or need to be updated for D13? It seems incredibly niche.

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 → .