#Bug Smash Initiative

Actually I want to make it less likely that a random test hook will collide with this and some more comments

Replied thanks!

idk if a good search but I looked for $definition['provider'] and didn't see any other spots that are doing translations.

Appears to have an open thread

Actually, #222 was also NW for the change record.

Thanks @mgifford. The current MR is a global change (MR 7458, started by @skaught).

I'm removing Needs issue summary update because I believe my changes resolved the points from #222.

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?

@alexpott what do you think about #34?

@kentr I'd say that we make this change globally. If it were only in core themes it would be confusing. I'd agree that this should be fixed in core, and as such the Gin maintainers were right in having it marked it Won't Fix.

Would be nice to fix this bug before it turns 13.

Closed one of the child issues as a duplicate and added the other issue.

Adding to saved credits.

Well, let's stick with "outdated" so that people don't go "what are they thinking?!".

I've never been so happy to see a terrible bug be closed without being fixed.

I gave @effulgentsia and @hchonov credit on the original issue instead, since there were a lot of other contributions there that had not been credited (major issue triage by the subsystem maintainers, various reviews, etc.).

🐛 Move entity preparation from form controller to entity class: "last changed" timestamp not updated and "create new revision" setting not respected when updating entities via Entity API Postponed: needs info was marked as critical by subsystem and core maintainers during a major issue triage for REST, so let's go ahead and promote this issue accordingly.

Also saving credit for various reviewers and discussion participants.

Adding credits for the triage from #34.

Wim Leers, gabesullice, effulgentsia

xjm → credited gabesullice → .

Saving credits according to our core issue credit guidelines → . Thanks!

Saving credits for creditable contributions to the issue.

It might be a dupe of #2559313: Parse composer.json for the project name for update module → at this point, however.

Titling to describe the remaining scope of the problem. As far as I know this is still an issue these days.

Adjusting saved issue credits according to credit guidelines.

Щас!

The question about what to do in calculateDependencies() with invalid data gets us into subsystem/release/framework manager review territory. As always, @mstrelan, a really great catch!

I can't decide which and I'm two of the three, so let's put these two tags on for now, to make sure we take a close look at the decisions we make about this before an eventual commit.

First, though:

1. Let's address the deprecation stuff to get that out of the way.
2. Let's get others' feedback/info on the questions from @mstrelan's point on calculateDependencies().

Then, we can decide exactly what to do about calculateDependencies() and what we need to addfor handling existing broken views.

Thanks everyone!

My last comment should have referenced #29. I edited my comment to correct.

Also adding STR to help with testing / reviewing.

Priority to Major because #21 tagged this with SC 2.1.1 and that SC is Level A.

I'm looking at the MR.

Can you please address the code sniff and php stan failures?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

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

This needs a reroll and a conversion to an MR.

Subsystem maintainer here. I think that the future in this problem space is not so much to remove the op as to refactor away from it being a single hook into dedicated hooks. I also agree that it is not a bug and way down the list of anything we want to try to do with the node access API. (API changes to node access should be limited to those required for forward compatibility or to replace it with a generic entity access API.) For now, reclassifying as wontfix. Thanks!

Saving credits for issue triage and other contributors.

Back to PMNMI then since we still need steps to reproduce this.

Took a pass at credit and updated the issue summary.

Scratch that this was fixed in 11.2 but it will be good to have explicit test coverage.

I'll clean up the issue summary etc later.

Needs a rebase, but I have a clean test that shows this is still an issue.

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

The rationale for closing this issue was mistaken. We are getting this on valid block configuration contextual menus in 10.4, for instance. No DDEV, no Provision module, none of the other peripheral issues raised in the thread. Just Drupal. Like many problems, "Tell me how to reproduce it from drupal install" is essentially impossible. But it is happening nonetheless.

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

Left some comments after reading #30 but am tagging for summary update as proposed solution doesn't seem to line up.

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

Great idea about reducing the strain on the localization servers @heddn.

smustgrave → credited acbramley → .

@acbramley for 🐛 Language attribute is not attached to node title on a full page view Active

Since there's been no follow up going to close out. If still a bug in D11 please re-open!

Thanks!

gaëlg → made their first commit to this issue’s fork.

Did a search for #[Block and there appear to still be other instances

Mainly test blocks, if those aren't needed can we least update the block.api

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

On Drupal 10 and 11 none of the patches fix this, ModerationStateField's methods are not even called.

Since I am having difficulty getting the issue reproduced (and the bug report in our project is a little old), submitting a branch with only the test changes to see if it is still broken.

I'm currently testing on Drupal 11.3-dev, and I’m able to reproduce the issue through the UI. After applying the patch, the issue is resolved in the UI as expected.
However, I wasn't able to reproduce the error reliably through automated tests. Given that, I'm submitting this MR with the patch and leaving the addition of tests to someone else who might be able to implement them more effectively.

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

I was able to reproduce on the last core version 11.2.2 so I don't think the issue is resolved.

What was the core version you tested on ?

I tried reproducing the original issue locally by manually duplicating a view display with an exposed form and simulating the faulty override (exposed_form set with defaults set to FALSE). However, the test I wrote passed successfully without applying the patch.
This suggests the issue might already be resolved in the current version of core.
Do we still need to proceed with adding the test, or is this no longer reproducible and can potentially be closed?

If just a docs update believe what’s proposed in #8 makes sense

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.

Updating IS tasks.

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

A transliteration subsystem maintainer signoff would also be good.

There is no issue tag for "Давайте послушаем наших русскоязычных друзей!".

I am also interested to know what Bulgarians think about this transliteration, or if Bulgarian maybe needs an override like Danish has? But that may be out of scope.

I dont mind marking but I also dont speak the language so maybe someone who speaks can review for accuracy?

lol, thanks Stephen. Updating the IS.

I agree and would also vote for bug, given that there's even a place in the condition plugin's code where you provide a dynamic summary to be picked up in this exact vertical tabs place.

So I think we only need the StatisticsLastUpdated field, filter and sort to entity types which implement EntityChangedInterface and have a changed field. That way we avoid quite a few issues. If you want to filter on the last_updated time in comment_entity_statistics then you can use the last comment timestamp field, filter and sort... you do not need this special implementation.

I just submitted the most basic MR implementing this idea, because I was running into the exact scenario of the duplicate search form. I did include the test patch, but nothing in the way of deprecation code yet. I did ponder it for a bit, but it is not evident to me how to deprecate this properly. Do we include a whole new function that people should call instead? If so, should this issue cover all core use of the old function?

@pandepoulus yep assigning the return $this->query->orderBy() to a property is pointless. I've updated the MR to fix this for 11.x.

We still need to add test coverage for the comments on entity types that do not implement EntityChangedInterface

#378 works for me. Thank you

Test-only test now fails as it should. Other tests in the pipeline are breaking. Not sure if related.

For core versions below 11.2 our projects are still using the patch from #309. However, that patch is not applying with every patching methods (it is failing for example when used with cweagans/composer-patches version 2). The reason is that since the patch was published, one of the test methods got a void return type. This new patch is a reroll of #309 that adds that void return type.

Sorry to be that guy can we update the issue summary with least proposed solution please. Will keep an eye for it to come back and rush review it

Fixed PHPCS. Rebased. Now several tests seem broken in pipeline.

Since there hasn't been a follow up I'm going to close out but am leaving all the credit assigned.

catch → closed merge request !12245

I think the overall assessment that this test never worked, can't test what it's trying to and should just go makes sense. Maybe we theoretically might have some test coverage missing, but that's already been the case for several years and we'd need to start writing the test again from scratch, in which case the current test does not help with that. Committed/pushed to 11.x, thanks!

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

Patch from current state of MR !11923

> Summaries are a javascript only implementation, there is no server roundtrip. Whatever we do, there is no way around each condition that wants to display a summary having to provide a javascript snippet for this.

I don't think that's true. The JS that creates a summary line is completely generic: it just takes the checkbox labels and joins them together:

      function checkboxesSummary(context) {
        const values = [];
        const $checkboxes = $(context).find(
          'input[type="checkbox"]:checked + label',
        );
        const il = $checkboxes.length;
        for (let i = 0; i < il; i++) {
          values.push($($checkboxes[i]).html());
        }
        if (!values.length) {
          values.push(Drupal.t('Not restricted'));
        }
        return values.join(', ');
      }

That would cover most condition plugins in contrib.

The only thing that needs to be changed would be the JS that decides which vertical tabs to apply the above to:

      $(
        '[data-drupal-selector="edit-visibility-node-type"], [data-drupal-selector="edit-visibility-entity-bundlenode"], [data-drupal-selector="edit-visibility-language"], [data-drupal-selector="edit-visibility-user-role"], [data-drupal-selector="edit-visibility-response-status"]',
      ).drupalSetSummary(checkboxesSummary);

Make it opt-out so more complex conditions that supply their own JS and declare that they're opting out in their plugin definition.

I call this a bug -- Drupal is meant to be extensible, and this isn't.

catch → closed merge request !11946

::hasDefinition() is great, solves the same problem without the new dependency.

Note I'm not crediting various commenters here who manually tested the issue and posted screenshots - there is no visual bug to verify here, it's a config issue that already has automated test coverage and is easy to reproduce, so going through manual reproduction steps on top of that doesn't really help to fix the issue. Issues that need manual verification are usually CSS or markup issues where it's not easy to write automated test coverage and will usually have the 'needs screenshots' tag.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

Hi,

I have tested this ticket on the Drupal 11.x, and working as expected.

Steps followed:

1. Create a New Menu Type (/admin/structure/Menus/Add Menus), Save.
2. Add the Menu to a Content Type (Structure > Content types > Basic page > Manage fields>Edit >Menu settings)
3. Select the newly created menu from "Available menus", Save
4. Delete the newly created Menu (Structure > Menus)
5. Check if Config is Still There (Configuration > Development > Configuration Synchronization),
6. Go to Export > Single item, Choose Content type, Select Basic page, Click Export
7. Now open the exported YAML and check

After applying the patch successfully, able to see the menu is removed from config when it is deleted.

Hence moving this to RTBC+1

Attaching the screenshot for the reference.

The weird thing moved from the collapsed third column to the uncollapsed second column, but it still isn't not a problem. It is still a strange and arcane secret behavior of a very basic configuration option for the very first page you see when you install Drupal. :)

acbramley → closed merge request !12337

xjm → credited alexpott → .

xjm → credited catch → .

xjm → credited star-szr → .

Adding credits for the triage from #8.

Thanks for that @nixou but changes need to be done in an MR.

It would also be great if you could put those steps into the issue summary using the standard template under the steps to reproduce heading.

Thanks!

Can we update the summary with a proposed solution.

Done

Also this brings up a question about attributes, if we are adding a new key how do we ensure contrib/custom modules update theirs too? WOuld have to be done in the attribute itself?

The key already exists in the attribute, but it's optional. We're ensuring all core blocks have a category set. Contrib/custom will fallback to the module name, which we're now translating. Although I was under the impression we're only meant to translate string literals, so not sure if that part is suitable. We could always deprecate not passing a category, but that could be pretty annoying.

(Of course, there's the whole thing where in Bulgarian it makes almost a "th" sound as I learned during Dev Days last year, but what can you do.)

I almost made the branch name 3215368-щ but then no one would have been able to type it.

Agreed; shch should be used.

Can we update the summary with a proposed solution.

Also this brings up a question about attributes, if we are adding a new key how do we ensure contrib/custom modules update theirs too? WOuld have to be done in the attribute itself?

I ended finally with this two patches (for drupal 10.4.5 and 11.2.2) for which the idea is the following :

1. As per #2340623: Views REST export does not support exposed filters → , both Rest and Data export should define that "they use exposed" but anyway they cannot store any exposed form display configuration
2. They neither can store which was their "original display in the duplication" and they do not really seem to need to know that
3. So to avoid these problems we could just say : if the exposed_form option seems overrided but the display does not has a key for the override, we will just get this information from the default display

The fix is global as it concerns all possibles options but I let maintainers say if that's ok or not or this a better way to fix it.

After further investigation, I found the following :

1. $exposed_form->preExecute() is call because $this->usesExposed() returned TRUE
2. $this->usesExposed() invoke RestExport::usesExposed() because DataExport extends RestExport and do not redefine usesExposed()

According to https://www.drupal.org/project/drupal/issues/2340623 → it seems necessary that usesExposed() return TRUE so exposed filters will apply on the export.
This seems to be confirmed by my test as if I put FALSE in the return, the admin/content exposed filters are not anymore taking into account in the data export.

So I was wondering how a REST export would handle this and I can see that the exact same problem occurs if I duplicate my page as a REST export.

This should confirm that the probleme is in core as we can reproduce even without Views Data Export.

I reopen this issue as I encounter the same problem and was able to determine the reproduction steps.

Drupal 11.2.2 fresh install.

1. Install Views Data Export
2. Go into the Content view : admin/structure/views/view/content/edit/page_1
3. Create a new page display and set it a path, then save
4. Go back to the initial page display : admin/structure/views/view/content/edit/page_1
5. Click on "Basic" near "Exposed Form"
6. In the "For" list, choose "This page (override)". Apply, and save
7. On the right in the select list "View Page", select "Duplicate as data export"
8. See that you immediatly get an error : "Oops, something went wrong. Check your browser's developer console for more details."

You can enable DBLog or check the log by another way and see that the error is : "Error: Call to a member function preExecute() on null in Drupal\views\Plugin\views\display\DisplayPluginBase->preExecute() (line 2367 of core/modules/views/src/Plugin/views/display/DisplayPluginBase.php)."

I'm going to investigate further but for now I understand that, as soon as you override the Exposed form configuration for a display, you obtain an entry "exposed_form: false" in the "defaults" part of the display :

display:
  default: [...]
  page_1:
    id: page_1
    display_title: Page
    display_plugin: page
    position: 1
    display_options:
      exposed_form: [...]
      defaults:
        exposed_form: false
      [...]

which means "This display does not use the exposed form settings of the master display".

When you duplicate this as a Data Export you get :

display:
  default: [...]
  page_1: [...]
  data_export_1:
    id: data_export_1
    display_title: 'Data export'
    display_plugin: data_export
    position: 1
    display_options:
      defaults:
        exposed_form: false
    [...]

which means the same thing : "This display does not use the exposed form settings of the master display" but in this case there is not the exposed_form additional entry.

For now I guess that the solution would be to not include the

display_options:
  defaults:
    exposed_form: false

part in the duplicated display if this one does not allow to use an exposed form or to not considere it in DisplayPluginBase.php.

Note that the provided patch is not ok as it will prevent to override exposed forms on displays (since the exposed_form: false entry is not exported anymore).

Thanks the check looks right but we should inject ModuleHandler here now.

I didn't do it initially because IIRC it's on the roadmap to split up these bulk Hook classes and wasn't sure if it mattered. Also, the procedural call is used throughout the other hooks. So I decided to err on the side of following the example of what was doing elsewhere in the class.

I took @berdir's suggestion of checking for the node_type definition so there's no new dependency.

catch → closed merge request !10528

Committed/pushed to 11.x, thanks!

Thanks the check looks right but we should inject ModuleHandler here now.

I addressed the feedback and made code changes.

Thanks for the response - replied on the MR.

larowlan → closed merge request !11204

Committed to 11.x - congrats folks, this was the oldest open bug in Drupal core, AKA 'the one ⭕️' in #bugsmash parlance

Published change record.

I addressed the feedback with no edits. Restoring RTBC.

Couple of questions on the MR.

catch → closed merge request !11546

This looks good to me now, the last few comments explain why it's a good idea at all, #3160721: Handle exceptions gracefully when saving entity forms → is open for making it generic to all content entity forms.

Committed/pushed to 11.x, thanks!

Although this is a bugfix it feels like a big enough change that it should probably just go out when 11.3.0 is released - if you strongly think it should be backported though feel free to re-open to discuss.

I added the change record. I haven't addressed other feedback yet.

I updated the patch to be compatible with Core 11.2.2+. Patch applied cleanly on my install.

Since there's been no follow up going to close out. If still a bug in D11 please re-open.

Believe feedback has been addressed regarding system_requirements

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

Made some changes to the CR, https://www.drupal.org/node/3525119 →

Fixed conflict with 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active and added check for setting in SystemRequirements::checkRequirements() instead of system_requirements().

This is now fixing cacheability in both views and regular list builders.

larowlan → closed merge request !11809

Committed to 11.x and unpostponed the follow-up - thanks all

Credits

Added 📌 [pp-1] Update use of Range constraint with min 0 to use PositiveOrZero instead Postponed as a follow-up to move other cases of Range w/ min 0 to PositiveOrZero - I agree with @borisson_ - it is more expressive