#Needs tests

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

The issue persists. Updated patch from #2 for the version 3.3.12.

Hi ,

I referenced another issue asking for complex support
https://www.drupal.org/project/inline_entity_form/issues/3228086#comment... 💬 Adding complex widget to custom config entity form Active

I pushed a rebased version of 3.0 on this branch issue , I do not know drupal git policies are okay with that

proceeding to some tests

Reviewing as part of Bug Smash Initiative. As per the comment in #25 marking this as PMNMI.

Yes, this is still relevant.

MR opened. I can see that the build looks like it will fail with an error that Drupal\date_recur_entity_test\Entity\DrEntityTest does not exist. I can say locally with a feature branch off of 3.2.x that this class does exist and Drupal\date_recur_entity_test\Entity\DrEntityTestBasic does not, which is why I "fixed" this in my MR. Not sure what's going on there.

Use form element of type date instead textfield when selecting a date in an exposed filter

And to be consistent we should use datetime-local element when selecting a datetime in an exposed filter.

Yeah, it's never been clear to me where this sort of thing should go. Right now we have an awkward combination of:

• $settings array in settings.php
• $config array in settings.php
• config form with a UI
• container parameter in services.yml
• feature flag module
• some new API? ✨ Add an API for feature flags Active

Also ideally it wouldn't be a moduleExists() check, instead the code to handle this feature would live self-contained in the contrib module, extending basic_auth as necessary.

All feedback appears to be addressed

Test-only gives

1) Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testExtraFields
Behat\Mink\Exception\ExpectationException: An element matching css ".block-extra-field-blocknodebundle-with-section-fieldlayout-builder-test-empty" appears on this page, but it should not.
/builds/issue/drupal-3152281/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3152281/vendor/behat/mink/src/WebAssert.php:492
/builds/issue/drupal-3152281/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php:492
FAILURES!
Tests: 16, Assertions: 187, Failures: 1, PHPUnit Deprecations: 17.
Exiting with EXIT_CODE=1

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Instead of having a settings.php entry, how about a submodule, e.g. "Basic Auth with Site Lock" (the name probably needs work) - this would act as the flag instead?

The feature flag module idea is interesting, but seems like potential overhead (haven't benchmarked how expensive calls to the module handler checks are) for a feature that most sites would probably rarely need.

I took inspiration with how the phpinfo() status page behaviour → was changed, and the setting seemed like the way to go for a functionality tightly coupled with the site's security.

However if that's the convention going forward for core to make it easier for non-technical people to work with it, then I have no objection.

Would need input from members on the core team to provide further direction on this.

@heddn:

I added that work-around to the issue summary. (Back when the issue-summary template included comments, it suggested adding work-arounds. I think it was under the "Proposed resolution" section.)

Any migration that is referenced by the migration_lookup or sub_process plugin is automatically added as a dependency. (See Drupal\migrate\Plugin\Migration::getMigrationDependencies().) I just checked: they are added as optional dependencies. So that should not trigger the problem in this issue.

There are still a few comments on the MR without any feedback. Also, for those facing this and don't want to apply a patch, you can always move dependencies from required to optional. I almost never use required because of reasons like what this issue causes.

It doesn't quite work for me

I have an on/off checkbox filter using BEF and with AJAX enabled

Using the MR it does remember/save the selection - if the box is not checked, I can check it and then reload the page and it stays checked, however if I uncheck the box it does the ajax request and then re-checks the box - I am then unable to change the state once its been saved from being originally checked

There is no other JS interfering with this button

@mark_fullmer

I performed the tests suggested in #197 and it was fine. I also tested in conjunction with redirect_bulk and csv upload of bulk redirects, and it works. Tested on Drupal 11.2.2 + PHP 8.4.

I have not set to RTBC owing to the reported failing of unit tests on the MR.

Given the strong demand for this feature, and the decision of the maintainers in #181 not to support it, I suggest creating a new project with a tagged release for the benefit of users who want the feature.

Now we have 🐛 There is no way to make a button element that can use the AJAX functionality Needs review in core, we could use this to actually create a button type="button" instead of using JS to change the button type.

Note that when applying #46 you need to save the config if you want to have "Update default configuration when new modules or themes are installed." option disabled (at admin/config/regional/translate/settings) as if it's not saved the value will fallback to true and won't have any effect.
If properly saved it works over 10.4.6.

@bernardopaulino thanks for the revised patch 2879648-path-alias-redirect-201.patch. It is giving me an error on Drupal 11.2.2, PHP 8.4.

TypeError: Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::__construct(): Argument #9 ($redirect_path_processor) must be of type Drupal\redirect\PathProcessor\RedirectPathProcessorManagerInterface, Drupal\Core\PathProcessor\PathProcessorManager given, called in ~/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in Drupal\redirect\EventSubscriber\RedirectRequestSubscriber->__construct() (line 97 of modules/contrib/redirect/src/EventSubscriber/RedirectRequestSubscriber.php).

@bernardopaulino thanks for the patch. It is giving me an error on Drupal 11.2.2, PHP 8.4.

TypeError: Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::__construct(): Argument #9 ($redirect_path_processor) must be of type Drupal\redirect\PathProcessor\RedirectPathProcessorManagerInterface, Drupal\Core\PathProcessor\PathProcessorManager given, called in ~/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in Drupal\redirect\EventSubscriber\RedirectRequestSubscriber->__construct() (line 97 of modules/contrib/redirect/src/EventSubscriber/RedirectRequestSubscriber.php).

@mikemadison: For example with PHPStan, you can ignore errors or whole files and directories: https://phpstan.org/user-guide/ignoring-errors#excluding-whole-files, so you would run your tool in CI with configuration that ignores the test files of Upgrade Status. (You would probably want to ignore tests of contributed and core modules in general to speed up your CI, even if you want to keep checking tests of your own custom modules).

Asked @larowlan to double-check my security concerns, and per https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... seems like he's +1 😅

> Cache tag isn't great since Drupal de-dupes multiple invalidations.

Yes, but only if there were no new caches set since the last invalidation, that's not an issue.

However, cache tags still need to be actually invalidated, and there are _many_ dynamic permissions based on bundles and other config entities, all those would suddenly need to add cache tag invalidations.

This is going to require a lot of work and it might be easier to deal with specific issues on a higher level. For example, the recipe action could support a list of permissions instead of saving the role config entity 30 times, that's always going to be slower.

But I would have thought PHPStan would have caught it...

PHPStan doesnt report on the issue to level 8. We are presently on level 9 + baselined strict.

167 Parameter #1 $account of method Drupal\Core\Entity\RevisionLogInterface::setRevisionUser()
expects Drupal\user\UserInterface, Drupal\user\UserInterface|null given.
🪪 argument.type

Happy for a follow up, automated ST is a edge case and I can see why tests might not have caught it.

But I would have thought PHPStan would have caught it...

On entity schema, not much we can do about that. Adding a dep wont automatically install schema, etc. Its one of those things you have to deal with on your own if youre implementing kernel tests involving third party Drupal code.

FYI this broke kernel tests for us on a module automating scheduled transitions in 2 ways:
1. User entity schema needed to be installed, it probably should have been anyway (ST also doesn't declare a dependency on the user module which should probably be fixed?)
2. $scheduledTransition->getAuthor() can return NULL if there's no current user setup, which in turn throws TypeError: Drupal\Core\Entity\RevisionableContentEntityBase::setRevisionUser(): Argument #1 ($account) must be of type Drupal\user\UserInterface, null given

I just ran into this again. I had a Recipe that applied 30 permissions. It took 2 seconds because the role validation causing `\Drupal\user\PermissionHandler::buildPermissionsYaml` to run at each granted permission. I'm guessing \Drupal\Core\Extension\ModuleInstaller::doInstall would need to reset cache on each install.

Cache tag isn't great since Drupal de-dupes multiple invalidations. But then the module handler is aware of the cache ID. Unless PermissionHandler has a specific "reset" method added.

Automatically closed - issue fixed for 2 weeks with no activity.

Anyone following this issue: 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active is now ready for review if anyone is willing to give it a look. Thanks!

Per Slack chat with @xjm, postponing this bug fix on 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active , which I've re-scoped into fixing UpdateMailTest before we try to expand it here.

Ahh, thanks @lendude, I missed #6:

I think the collapsed “Advanced” column was a rather brute-force way to reduce clutter in the Views UI. I wouldn't mind showing it expanded again.

Might be worth a follow-up to try to remove the overrides, then we wouldn't need to update them in Drupal 12 except for when we delete them.

There are a lot of them:

core/modules/block/src/BlockListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/block_content/src/BlockContentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/comment/src/CommentTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/field_ui/src/FieldConfigListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/filter/src/FilterFormatListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/image/src/ImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_ui/src/MenuListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/node/src/NodeTypeListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/responsive_image/src/ResponsiveImageStyleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/search/src/SearchPageListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/shortcut/src/ShortcutSetListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/taxonomy/src/VocabularyListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/user/src/RoleListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/views_ui/src/ViewListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {

Two content entity list builders too:

core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {
core/modules/menu_link_content/src/MenuLinkListBuilder.php
  public function getDefaultOperations(EntityInterface $entity) {

They do various things, so I don't think they can be removed.

Of relevance to this issue, these implementations of getDefaultOperations() include access checks inside them, so I wonder if cacheability needs to be captured.
core/modules/field_ui/src/FieldConfigListBuilder.php
core/modules/taxonomy/src/VocabularyListBuilder.php
core/modules/workspaces/src/WorkspaceListBuilder.php

The last patch was a mistake.
I uploaded the new patch that works against 11.2.x

Since no Drupal 7 versions are still supported, this can be closed.

Updated patch from #53 so it applies on D11.2.

Finished reviewing the MR. Epic work! 🤩 Confirmed that it meets all 5 goals outlined in the issue summary! 👍

My commits/self-addressed:

• Comment nits.
• A fix for using root-relative URLs for the example images rather than absolute ones

Remaining feedback:

• Spotted a few bits of missing test coverage.
• Everywhere this MR uses "default", it should use "example", because of 🐛 DX & authoring experience: for SDC+code components, the example is treated as the default, may not be desirable Active . (Yes, I know this is how we've been talking about it for a while, but that doesn't mean we shouldn't realize our mistake and get better. Not your fault, but this MR understandably introduces a lot more uses of this terminology! So: time to rectify it now.)
• Concern: manually creating a File entity in a test — what am I missing?
• Major concern: the way this paves the path for ✨ [PP-1] Add support for example images for Code Components' image props Postponed is nice but AFAICT insecure 😅 — I think it actually might be better to REMOVE this bit from the MR, because of the security aspects, but also because it's not in scope for this issue (for that very reason).
• In fact, a similar concern exists when just using encoded_files itself; a config export+import could become an attack vector. See https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... + https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... + https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Thanks, @larowlan for creating 📌 [PP-1] Consider adding ramsey/uuid for hash-based UUID (v5) generation for default files Postponed: needs info — linked to it 👍

Thanks for the excellent docs addition, that made it 10x easier to review this! Still in the process of doing so.

Created ✨ [PP-1] Add support for example images for Code Components' image props Postponed for the actual feature that this is intended to unblock.

Thanks for creating the MR @karengrey. We've been using the various patches in production successfully for years, however the issue is still tagged with NeedsTests so I'm going to set this back to Needs Work for now. Hopefully we can get some tests written and get this issue wrapped up.

Rewriting the patch.
Providing a patch for version 5.2.x for those who wish to use MR-164 with an older version.

Thanks.

Any chance of a backport to 2.7?

Okay, did some more sleuthing, and decided that 2 of the existing cases in this test are totally bogus, as is 1 of the new ones we added:

• _update_cron_notify() is the only place sends the 'status_notify' email.
• It returns early if there are no values in the $params array.
• It therefore seems pointless to unit test our hook_mail() implementation without parameters.

Removed all 3 cases, and the related @todo comments from my attempt to document everything.

Apologies for the possible scope creep. Perhaps we should postpone this on a new follow-up just to fix all the brokenness in this test, then circle back to fix the bug and expand the test. But it seemed more prudent to fix them all in 1 shot.

With the draft CR done, all MR threads resolved, and a green pipeline, this is ready for review again. 😅

Initial CR: https://www.drupal.org/node/3538659 →

Do you know why?

It looks like it might have been accidental. See the following commit where public function getOperations was changed to public function getDefaultOperations

https://git.drupalcode.org/project/drupal/-/commit/b504423ed07e9bb437e96...

Added 🐛 getDefaultOperations() in ConfigEntityListBuilder and sub classes should be protected Active

This is getting close. Pushed a few commits to resolve a few more of the open threads. Down to 1 (the request for inline comments describing each case in UpdateMailTest::providerTestUpdateEmail()).

Also, opened 📌 Remove $authorized param to UpdateMailTest::testUpdateEmail() Active which I noticed while reviewing this. That's totally out of scope here, but should happen.

Also related: 🐛 [node:summary] token's value get trimmed incorrectly Needs work

This is related: 🐛 [node:summary] token's value get trimmed incorrectly Needs work

This is still an issue for 11.x
Have created a MR based on the patches created previously.

#295 applies and works for Drupal 11.2

@bedir you're seeing a 500 error on finalize with MR14? If so, what line did you end up commenting out?

Instead of having a settings.php entry, how about a submodule, e.g. "Basic Auth with Site Lock" (the name probably needs work) - this would act as the flag instead? The module description can explain why you might want this enabled if you are using Basic Auth.

> The basic auth provider will only act upon routes which have been explicitly tagged with the basic_auth _auth option.

If basic_auth is enabled, and basic auth is received on a route that is *not* tagged for basic auth, should we log or display a warning message?

That's an accidental entry to the issue summary, and actually meant for the 🐛 Basic auth returns 403 when username & password supplied but not needed. Needs work issue.

The changes so far only check for the username's existence.

But no, I don't think it's worth logging it as modules making use of basic_auth should already be tagged as such, and bad actors can use this as a means of filling up the logs without suffering the penalty of being banned by the flood control system.

> The basic auth provider will only act upon routes which have been explicitly tagged with the basic_auth _auth option.

If basic_auth is enabled, and basic auth is received on a route that is *not* tagged for basic auth, should we log or display a warning message?

I agree with @xjm this isn't really a bug, so moved to task, might be a feature ¯\_(ツ)_/¯

@dawehner seemed to be in favour of this back in the day, and so am I, so removing the subsystem tag. I understand the initial thought behind not showing this, but I believe the Views UI is still plenty complicated without showing this column. And as people have pointed out in this issue this can lead to information being hidden. Also, it always felt a little arbitrary what was considered 'Advanced', since, lets be honest, the whole Views UI is pretty advanced :)

@yoroy has commented here so not sure if a usability review is needed, but happy to follow @xjm on this, so tagging for usability review

Added the username check on the basic auth prompt (hidden behind a setting) and a simple test case for it.

codebymikey → changed the visibility of the branch 11.x to hidden.

There's still an open discussion here about moving to a service parameter instead of using $settings.

rerolled patch against 11.2.2

As found in v2.8.2

Merged!

Thanks all.

Closing as outdated.

There have not been other reports of this issue, or followers, in the last few years.

ConfigEntityListBuilder and all its subclasses override this and make getDefaultOperations() public. Wild.

Do you know why? Might be worth a follow-up to try to remove the overrides, then we wouldn't need to update them in Drupal 12 except for when we delete them.

All the list builder classes will need to have this parameter added in D12, but not sure if all the list builder classes need to be updated now to indicate the change?

It would probably help anyone subclassing the subclasses if we did that, I don't think we've had a similar situation yet - the commented out added parameters is pretty new in itself.

I didn't see @mohit_aghera added tests, thanks.

Thanks for that suggestion @mstrelan, looks good.

Just a minor nit I've added, otherwise this looks good.

See the first part of this MR https://git.drupalcode.org/project/drupalorg/-/merge_requests/385/diffs done for #3164818: Timezone issue with community events → .

Excerpt:

      // Capture current timezone.
      $script_tz = date_default_timezone_get();
      // Set timezone of the DB to calculate timestamps.
      date_default_timezone_set($event_dates['timezone_db']);      
      $timestamp1 = !empty($event_dates['value']) ? strtotime($event_dates['value']) : NULL;
      // Once the timestamp is correct, use the timezone in the field to render the date.
      $date1 = format_date($timestamp1, 'medium', '', $event_dates['timezone']);
      ...
      // Restore script timezone.
      date_default_timezone_set($script_tz);

Dates are stored using the database's timezone, so we should use that one before any call to strtotime. Once we have the correct timestamp, then we can calculate the rendering with the field's timezone.

Note sure if the above will be relevant to the final fix but just wanted to give some additional context of how we are "bandaiding" d.org

I double check the changes, and achieve the same result as i found at #59
(Sorry that i didnt added the screenshots back then)
The advanced column "is gone" as the collapsible behavior.

So i will add the Before Screenshot that @uesli give to us to not flood the IS with duplicated images

Before - just checked with a clean D11.2-dev instalation.

----------------------------------

After - just checked with a clean D11.2-dev instalation + MR 11424.

This came up as a daily BSI target

The function in question seems to have been removed, here's the CR https://www.drupal.org/node/3032541 →

Can we confirm still an issue?

Updated issue summary with plans to move forward with this.

Thanks, both of you, looks great! 🤩

Tested on D10.5:

- Without patch entity browser window is opened by pressing Enter on any text field
- MR2 installed, tested, fixes the issue

Please release?

I had a look at this last week in a build, possibly no longer using this. I couldn't find it in my builds.

Dumb question but see $this->container->get('entity_type.bundle.info')->clearCachedBundles(); is needed to get tests to pass now. Could this be a breaking change for a number of contrib tests?

Actually, that seems like a smart question. It's been a hot minute since I worked on this issue, but I think the problem can occur if a test creates a bundle at run time, requiring a cache clear. It's undoubtedly a possibility.

Dumb question but see $this->container->get('entity_type.bundle.info')->clearCachedBundles(); is needed to get tests to pass now. Could this be a breaking change for a number of contrib tests?