#Bug Smash Initiative

Fixed typo.

ivnish → closed merge request !8754

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

catch → closed merge request !12575

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

🌱 [Meta] Make Drupal the first "design-system native" CMS + Unify & simplify render & theme systems Active has on its list getting rid of theme suggestions altogether. I'm not really sure how that fits into things but just noting that they potentially could end up on their way out.

Well, this is awkward, I learned something today. And I keep running into problems here.

By renaming field--comment-body.html.twig to field--comment--comment-body.html.twig, what happens is that this is now triggering preprocess_field_comment functions.

The actual theme suggestions have no assumptions about depth and hierarchy, it's just first match in reverse order wins. Turns out that preprocess discovery doesn't work like that at all. It splits up the template name by "--" and then assumes that earlier parts are it's parents/hierarchy and should be called too. So because the entity type is comment, and the field type is comment, we're running into a different kind of collision.

I've added a workaround to the olivero preprocess, which is essentially the same as comment already has. This is an existing issue and not caused by this directly (except that you have to name your template now in a way to trigger this) But it also kind of defeats the purpose of this issue.

Makes me wonder if we maybe went too far in our approach to preprocess functions and whether it was a good idea to support preprocess for theme suggestions. But that's unlikely to change.

The only other options is that we do in fact rename the field--$type suggestion to field-type-$type. But that's going to cause so many BC issues (preprocess_comment will stop working).

#28 should of been moved to new status.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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. It no longer applies to Drupal core. 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. It no longer applies to Drupal core. 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.

All remaining deprecations were triggered by field--comment-body.html.twig in olivero, renamed that, so if the RNG gods are with us, this might be green.

Published the related CR

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Also opened 📌 Remove ConfigEntityListBuilder::getDefaultOperations() override Active .

catch → closed merge request !12445

This looks good, it's tricky with the bc for added parameters but at least we have a way to do it. Did my best with issue credit but this is a long issue with a lot of contributors so may have made mistakes.

Committed/pushed to 11.x, thanks!

#38 sounds like a very nice way of handling this, but I also am unsure whether there's going to be weird edge cases. However even if there are, it might be less disruptive than a complex renaming situation overall.

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.

I had an idea that I'm not sure is a good one, but it seems to work. Basically, I was thinking that we add some magic to the deprecation logic and if we discover a deprecated suggestion and the next in line is identical, we just skip to that. This means we basically assume that field--comment.html.twig is meant to be about the field type.

I can't think of an edge case where this would be a problem right now, but there might be?

acbramley → closed merge request !11090

Well, my suggestion didn't account for the fact that the default field name of the comment field in \Drupal\comment\Tests\CommentTestTrait::addDefaultCommentField() is 'comment', which is exactly what triggers the conflict. it's a comment field, so it's not broken, but of course the deprecation doesn't make a difference between that.

And actually, the default comment field in standard install profile is comment too.

Might need to rename the field type based suggestion after all...

Created an MR as a starting point.

After looking at this, I'm proposing to *not* deprecate and rename field--FIELDNAME.html.twig. That would require us to rename several existing templates, specifically field__comment.html.twig in comment module and it's 10 overrides and the new specific preprocess function and that seems like way more trouble than it's worth. That just means we can only remove that extra check in 📌 Investigate preprocess_field in the comment module Active in D13, but I can very much live with that.

The MR should fail with deprecations on field--comment-body.html.twig in olivero, all others are field type specific or entity type prefixed I think.

4 years later...

I think this is not a duplicate, but the meta issue of that or maybe better, a policy issue on whether or not we should do prefixes, it also references the entity issue that was essentially blocked on this.

I did make a specific proposal with a few questions in 🐛 Incorrect order and duplicate theme hook suggestions Needs work and would appreciate feedback there, but I think this is still an issue that should have its own resolution on whether or not we use prefixes or not, and if all entries should use prefixes or only less common ones, likely guided by the specific examples in the field issue.

I think #18 is valid not just for menu but this issue in general, that seems like an issue with explicitly specifying a suggestion. Closing as duplicate again, this time of 🐛 Incorrect order and duplicate theme hook suggestions Needs work

This is a lot trickier than the views issue we we just deprecated the suggestions completely. Order in general is not something we can *directly* provide BC for, but we might not actually need to...

The issue summary is very old, unsurprisingly and outdated. multiple entries are no longer there like that, specifically not bundle-only suggestions.

This is the current list:

$suggestions[] = 'field__' . $element['#field_type'];
$suggestions[] = 'field__' . $element['#field_name'];
$suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#bundle'];
$suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#field_name'];
$suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#field_name'] . '__' . $element['#bundle'];

(note: this is reversed as the last entry is the most specific)

one issue that's easy to resolve is $suggestions[] = 'field__' . $element['#field_name'];, field names are no longer global, so this should not be a thing and we can deprecate this. field type is global and then unique but also least specific, that's fine. However, because we only deprecate the suggestions here, you will only be able to rely on the field type once the field name suggestion is actually removed (D13). Including in core. One option is that we introduce a prefix for the field type and make that $suggestions[] = 'field__type_' . $element['#field_type'];. But this would force everyone to rename their field type specific templates.

This also solves all order problems. entity type + field name is more specific than entity type + bundle.

The next problem is the conflict of entity type + field name/bundle, meaning a field name and bundle of the same name. That seems less common but possible. I wonder if we should just straight up deprecate entity type + bundle. Is it really a common use case to have a template for *all* fields of a given bundle? If someone wants that, they could still introduce that on their own again.

Alternatively, we need to introduce prefixes for these two, so $suggestions[] = 'field__' . $element['#entity_type'] . '__bundle_' . $element['#bundle'];. This is what #2831144: Bundle / field name / view mode theme suggestions can conflict → was discussing. Which I think is not a duplicate but a parent/meta issue of this and the entity one. Either for both bundle and field or one only bundle, as that's likely far less used.

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

I've attempted to work on this a little and produced the following based on 11.x:
https://git.drupalcode.org/issue/drupal-2875033/-/commit/41ac87795f50465...

(Up front disclosure - I tested this as a patch on 10.4.x and this is just a cherry pick for analysis)

Here is an outline:
* For testing, a clone of a Query will now properly clone the conditions and update the attached query on the Conditions. Happy to take this out of this issue into another if it's desired. Essentially at the moment __toString on a Query is going to work with Conditions that still refer to the original Query. It causes problems in debugging mostly.
* getTables on Query was creating a new tables instance, not returning the existing property value. This meant tables added via separate Condition instances (such as nested in an AND or an OR) were not deduplicated - they were effectively creating new joins for every branch. This change keeps getTables for BC but it should be now unused, and it introduces getSqlTables that returns the same object after creating it so all conditions share the same instance, preventing attaching the same table twice
* Specifying different langcode in conditions for a shared table field previously could cause issues as it would reuse the same table join even though it would refer to a different langcode - now it joins separately for each langcode
* When following a reference field, the tables were never shared at all by addNextBaseTable, and this introduces a nextBaseTables property to track and reuse these
* When there is a data table, we don't just forcefully set simple_query to FALSE. Warrants some eyes and testing but it feels this is unnecessary as the table joining code that attaches the data table already sets it to FALSE at the point the table might get attached. In this change it actually only sets simple_query to FALSE if the join of a data table is not going to use a langcode. If a langcode is used, then the data table will provide a single row, simplifying the query.
* Added ability to specify langcode as "__default__" to make it join the data table on default_langcode=1 with simple_query remaining. For me this looks to massively improve query performance and allow us to add indexes in several places on the data table for massive gain. The API here might warrant discussion.

A note worth mentioning is that if a table joins as INNER JOIN and the next request is for a LEFT JOIN it will not change it. It will remain as INNER. This is how it was previously when deduplicating of table join was working. This seems fine to me as if it's INNER there's a restriction so loosening it serves no purpose. For the inverse, where it is initially added as LEFT or LEFT OUTER, it also won't change it to INNER if a request comes in. It seems fine to me this as I think from what I can see every join that uses INNER will have a WHERE addition anyway, so the fact it's LEFT doesn't make much difference. It could be "nice" to clean it up to INNER but then you're modifying already added tables in the query and it feels the API change stretches too far.

Generally it looks like the improvements from this experiment are:
* Tables are no longer duplicated in all cases (from what I can see)
* Where multiple shared data table fields are used in conditions, indexes on the data table can now get properly used as they use the same table join, and the optimiser can kick in
* Grouping is now completely avoided for basic queries that don't involve the shared data table
* Sorting by a shared data field that is untranslated can also be massively sped up with a quick index addition and specifying the langcode "__default__" as this ensures it keeps simple query and works on the default langcode, which inherently will have the untranslated value for that field. For me personally this is what I was trying to get to to resolve some slow queries.

Would be great to get some feedback while I do some further testing and look at adding some tests etc.

In Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → . Thanks

Hi, I found a bug using this patch and geofield at the same time. This patch add (['value'] = '') to the variable $gefield_item to improve the performance. However, this value cause that it doesn't enter in this if condition: URL

It can be solved with a small path, adding also $geofield_item['value'] == '' to the if condition, so is not a big deal but I thought it could be consider in the future for new updates to this issue.

Apart from that, the patch works well. Thanks for providing this awesome optimization.

(function ($, Drupal, bodyScrollLock) {
  'use strict';
  Drupal.behaviors.foo = {
    attach: function(context, settings) {
      $(window).on('dialog:aftercreate', function(event, dialog, $element, settings) {
        bodyScrollLock.unlock($element.get(0));
      });
    }
  }
})(jQuery, Drupal, bodyScrollLock);

You can disable it like this. In our experience having this enabled has created major issues in IOS with users unable to scroll dialogs that are larger than the viewport. It might be specific to our use case and we have not look at it beyond this yet.

@smustgrave I have updated the IS which explains and highlights the need for the patch file to enable manual testing. I have also added extra details on where this bug is being encountered.

So I tried to follow the steps in #8 but the tags field is using radio buttons so something is always selected. Are there other steps?

Wanted to bump 1 more time.

We need to repair a test in LinkNotExistingInternalConstraintValidatorTest to test this properly. This issue is postponed on #3542967: Bad test in LinkNotExistingInternalConstraintValidatorTest → .

I agree @lendude, a dynamic list would be better. The Issue Summary included this sentence, but at the end of the last item, so a bit hidden, and not totally clear:

If possible scan the web root folder and list the actual paths? Because they may change ...

I have updated the Issue Summary to emphasize that a dynamically built list would be best.

Agreed with the latest comments: as @pameeela suggested, if settings can be moved out of the UI would help not increasing Drupal's complexity. And since several of these assumptions don't follow a specific or generic UX rule and can change based on project & form specific case (but are necessary decisions to be taken) a setting in code is the best solution.
Just make sure the work happens on the right issues as @quietone suggested: specific and focused changes have better chances to get in, faster (it took me 30min to understand what happened in this issue).

Not 100% sold on the manual list of reserved paths, now when we remove 'profiles' from core for example, we need to remember to update this list (we won't). Is there no way to get this list dynamically?

And another observation: Even on /admin/config/search/path if I hover or click the URL alias for the 1st page then I will see the URL alias for the 2nd page - so maybe this issue isn't related to Views core module, but to another core module (like Path) or another basic core functionality...

I updated the issue summary because I can reproduce my reported problem still for Drupal 11.

If I can't re-open this issue I will create a new one.

MR still applies and don't think my comment should hold things

Left some comments on the MR.

Also CR was last updated in 2021 so could use some updates probably.

Still appears to need an issue summary update

I apologize: I see that my comment in the previous issue can be the cause of some misunderstanding. (I used eventually because it is a false friend of eventualmente, but that is not the word I should have used.)

The purpose of 🐛 AccountProxy should not be serialized Closed: outdated was never to change code comments, and it does not explicitly say which comments should be changed and how.
My comment was means to say that, if there are code comments that should be changed to reflect the fact the AccountProxy class now uses the DependencySerializationTrait trait, that could be done in a different issue. I have never said which comments should be removed, nor which comments should be changed, so a reference to my comment does not make clear what this issue expects to obtain.

I think the change done in this merge request is correct, but the issue summary needs to be made clearer.

Thank for the review smustgrave!

Resolved one comment and added justification for the new test class in a reply on the other comment. Back to NR.

Left 2 small comments on the MR.

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!

This came up as the daily BSI target (again!)

Do the patch from #24 and tried to address the comments in #25 and #29, seems package_manager is failing currently.

Thoughts?

I rebased the MR on 11.x, I had to drop some commits and reverts that were causing conflicts so it'll be easier to rebase later.
We were previously using 🐛 Access denied to published private file if original translation is unpublished Needs review on our project but I can confirm this solution also covers it.
Also attaching static patch for composer.

smustgrave → credited dcam → .

@dcam for 🐛 Undefined offset 1 in the MenuParentFormSelector.php Needs work