#Bug Smash Initiative

Adding a tag.

@norman.lol :

In which class do you propose adding this method?

Since TrustedCallbacks → , sth like this does not work anymore:

    $element['#pre_render'][] = [HelpCallbacks::class, 'preRenderHelp'];

Fix is:

    $element['#pre_render'][] = HelpCallbacks::class . '::preRenderHelp';

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

mohit_aghera → changed the visibility of the branch 2637808-languageformatter to hidden.

Hello @everyone
I also tried to change the title using git (by changing the commit message ), but sadly, that is not working, and I don't have access to change it directly at MR

The update function number was updated per the discussion in Slack. I also rebased it against 11.x to get the latest changes.

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

Rebased against 11.x, removed the ->addCacheableDependency($node) addition (out of scope), and moved testing to a dedicated test case since we need to disable the use_admin_theme setting.

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

acbramley → changed the visibility of the branch 9.2.x to hidden.

So while I was looking at the history for this issue to revert the changes made, I ran into this issue's bug. I viewed a comparison, then hit back and then tried to view the comparison again and the button didn't work until I reloaded the page.

Why did you overwrite all of the changes made to this issue?

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue based on @smustgrave's triage. We should focus on the issue summary update based on recent work and resolving the code review.

Adding a static patch file here for the latest merge request changes. The type declarations added from the previous merge request commits for the "$title" property was breaking our site because the property could also receive an array as its value.

I ended up using this solution on the specific page experiencing this issue to avoid any possible issues on other forms. Thanks for the assist, @rhovland.

Hi,
I could not find 10.4.x compatible patch for commit 3cf003db
so I made my own, I hope someone else finds this useful.

That feels like overkill - it's clearly fixing a common mistake most people do when adding css classes to render arrays: are we really going to add overhead to say this specific one has been added correctly?

Yes needs a test to show the issue.

Any reason why this has been set to needs work @smustgrave? There are no comments to advise what needs doing, and it's a very simple bit of code: I can't think what additional changes are needed?

Hi @joachim,

I’ve updated the comment to reflect that setRebuild(TRUE) is what triggers the behavior, as you suggested, while preserving the original intention of explaining how to get this behavior from a DX perspective.

I also rebased the branch onto the latest 11.x since it was many commits behind, to ensure the pipeline runs accurately.

Let me know if anything else needs adjusting!

We have a hook_node_links_alter that is called via NodeViewBuilder::renderLinks. Modules could use this to add the Read more or any other link to any view mode they like.

This avoids core having to do any work or upgrade path.

What if non teaser view modes were showing links for other purposes? Now they'd need to alter out the read more link.

I think this should probably just be closed as works as designed.

Change record added at https://www.drupal.org/node/3515212 → . Leaving status as Needs Work due to the question of what the update number should be. I checked Slack and no one has responded yet. I'll try to check back in a day or two.

Hi @quietone

The previous MR (!8124) was quite outdated and significantly behind the 11.x branch. I initially attempted a rebase, but due to a large number of conflicts, it became difficult to manage cleanly.

To ensure a clean and conflict-free update, I’ve created a new branch and opened this fresh MR (!11606) with the necessary changes, fully rebased with the latest from 11.x.

All tests are now passing. Let me know if anything else is needed!

Thanks!

If this is a bug, as per the IS, we need steps to reproduce.

Failing AssetAggregationAcrossPagesTest seems unrelated.

Asking about that update hook number (small change if needed)

But we should have a CR for the configuration change. Mentioning why it's been removed.

Don't know if I follow the task, if this still needed? Should it be moved out of PMNMI?

Very nice to see we’ve come so far that we can deprecate that! 🥳

Since there hasn't been a follow up going to close out but if still an issue in D11 please reopen.

https://git.drupalcode.org/issue/drupal-2973515/-/jobs/4680214 shows the test coverage and the actual fix seems pretty straight forward to me.

Change applies cleanly, test pass and the correct dependency is now added to the exported config. I think this is OK to go.

Not 100% sure about the message being set - see comments on MR.

Possibly unrelated to this issue, but this change would effectively make the "create term if it doesn't exist" setting impossible to use on any term vocabulary that had required fields: you would never be able to create the terms as the validation would always fail but the edit form has no ability to let you set the field values on the new term.

My personal preference here would be the current behaviour - stub taxonomy created that requires future updating - so that the setting on the field actually has a function. I can understand that not everybody would, however: perhaps we need some kind of validation on the field settings form that advises terms with required fields can't use this setting (if this change is ti progress)? Or at least a warning in the help text?

Looks good to me, and the random test failure has been resolved by a re-run.

sivaji_ganesh_jojodae → changed the visibility of the branch 3514936-modulesuninstallform.php-assigns-string to hidden.

Steps to Reproduce:

1. Install a standard Drupal profile.
2. Add a required field to the Tags taxonomy.
3. Ensure that the Tags field on the Article content type is configured to auto-create new terms.
4. Create a new Article with a tag that does not exist.
5. The new term is created with the required field empty, leading to data inconsistency.

Here I have attached After fixing Screen shot

After fixing →
ScreenRecordingofErrorthrowing →

Also needs a reroll against 11.x on an MR.

#90 still needs actioning and this needs to go into an MR.

It might be a tricky reroll as well since update fixtures will need fixing up.

I converted the patch in #2 to an MR. Then I fixed it because the return value from calculateDependencies() was incorrect. I added a test. At some point I realized that this needed an update path. So I wrote that and added a test for it too.

I did not address the question in #13. I wasn't sure if it's necessary since plugins are @internal.

We have one failure which I can reproduce locally NodeAccessLanguageTest::testNodeAccess

I think this is something weird with the caching changes, the lines just before the failing assertion set the catalan nodes as "accesible" via some state and then check that they're still not accessible based on static caching:

    // Make Catalan accessible.
    \Drupal::state()->set('node_access_test_secret_catalan', 0);

    // Tests that Catalan is accessible on a node with a Catalan version as the
    // static cache has not been reset.
   $this->assertNodeAccess($expected_node_access_no_access, $node_public_ca, $web_user);

Since we're testing the delete operation last based on $expected_node_access_no_access I expect that's nuking the static cache or something? In any case this seems like fragility in this specific test rather than an actual problem in the solution.

Rebased onto 11.x and made some minor changes. Updated IS and title to match solution.

acbramley → closed merge request !2074

Still valid, I tested #20 i.e doing Html::normalize at runtime in template_preprocess_node_add_list but it didn't work, I have no idea why though, debugging into the preprocess showed valid markup in the description variables. (EDIT: claro_preprocess_node_add_list also needed the fix)

Also, these descriptions are shown elsewhere such as on admin/structure/types so I don't think it makes sense to do it at runtime.

Can we add an issue summary?

This code was added when core either didn't have JavaScript testing or didn't have very much of it.

Reading this, I'm wondering whether we should look at removing this altogether - form.js is not included for every form, it's included only when the library is added, and none of the places in core that include it specify that it's for double click prevention. So we have some forms with double click prevention, and some without, pretty much at random. There is no per-form opt-in or opt-out, so if the js is loaded on the page for any reason, it applies to all forms, not just the one that loaded it. Some of this was pointed out in 🐛 Double click prevention on form submission Fixed after commit too (although the history of that issue is hard to track because it was then moved to Drupal 7 for backport).

I think we should probably discuss whether to remove it altogether before trying to add test coverage, but given this can completely break forms if there's an issue, we'd probably need to add tests both for the original double-click prevention and the back button situation if we go ahead.

As the original reporter of this issue, I only faced this issue on one project and I haven't been able to reproduce it since.

As far as I am concerned, I am OK with closing the issue unless someone else is still facing it and can provide more information.

charlliequadros → changed the visibility of the branch 2873447-views-rest-export to hidden.

I attempted to address the issue summary. Can you please let me know if there needs to be further changes to it.

In regards to a test, where do you suggest it be placed? It seems FunctionalJavascript tests for forms are located at core/tests/Drupal/FunctionalJavascriptTests/Core/Form. Should a new test file be created there? Should it be added to an existing Form test?

When writing the test what am I testing for? Am I writing a test to ensure that the submit button is disabled when clicked twice? And then navigate back and ensure that it is not disabled? I was unable to find an existing test for the double click prevention feature.

Lastly, is there a way to ensure someone with more experience with javascript reviews the merge request for correctness? I'm uncertain if I did it correctly. There's also a failure of a nightwatch test that I don't really understand (I've never used nightwatch before).

Appreciate your feedback, I'm still learning and have only done work with tests in contrib modules.

Based on 🐛 Fatal error "getCacheTags() on null" on admin/content Views page Closed: works as designed being opened I am reopening this. It still needs steps to reproduce. 🐛 Fatal error "getCacheTags() on null" on admin/content Views page Closed: works as designed doesn't have any.

I am applying the patch in #20 to this contrib version of tracker, it works without issue.

It is sorta a bug in that you can add the field with no comment type available. There should be a type of check in that space.

I've tested the patch by @rhovland on the checkout screen (Commerce 2.36) and it works well. Thank you!

Game up as a BSI target

After 7+ years wonder if still an issue with all the changes to views from 2018 to now?

Thanks for reporting.

Issue summary needs to be complete, especially steps and proposed solution sections.

Also will most likely need a test showing the problem.

Duplicate of 🐛 Toolbar overlap content with big pipe enabled Needs work ?

Rerolled

Thanks for this great patch. I've attached the modified patch to also affect JavaScript.

We need a proper update to the IS and title here. Also it sounds like this might be fixed by ✨ Enable an entity query's return value to carry cacheability Active ? Is this a duplicate?

Asked in slack about how to remember to add the type hint in 12.0.0 when the trigger_error is deleted. @larowlan suggested a new issue. The issue is 📌 Add type hint to RefinableCacheableDependencyTrait::addCacheableDependency Postponed .

I'm not so sure we should do this, once we swap to the generic revision UI in 📌 Switch Node revision UI to generic UI Needs review this would no longer work and I'm not entirely sure it's necessary to override the title in this way.

Further, the current solution changes both the page title and the title in the rendered content, making for a bit of a weird display.

Rebased onto 11.x and tidied up the test a bit.

I wrote an event listener for form.js that unsets the double submit data if the page is loaded via the cache (user hits back button), allowing the submit button to work as expected.
If core wants to prevent ever resubmitting a form then it should be a configurable option not a blanket form API feature.

As written this prevents the problem from the original issue, clicking submit multiple times, while avoiding breaking forms when the back/forward buttons are used.

In my testing it affects both Firefox and Chromium.

This affects all submit buttons in a form not just the one that was previously clicked.

So for an example I have a cart form. It has multiple submit buttons. The main one is the checkout button. There are also remove buttons on each cart item and an empty cart button. None of these buttons are functional after clicking back in the browser after submitting the form.

This is the issue where this feature was added
🐛 Double click prevention on form submission Fixed

The feature was intended to prevent double clicking a submit button causing two form submits from happening. It was not intended to prevent the same form from being submitted again after loading the submission page and going back in the browser.

This feature as written causes unexpected behavior in forms across core and contrib. There is no mechanism to opt out a form from this protection. The only way for an end user to fix the page is to refresh the page or change a form value (if they can).

I've created a MR with the changes of #79 to apply against D11 or D10.4. I've tested the patch locally and it works avoiding to display terms alreay applied when using an entity reference field with the widget "Autocomplete (tag style)"

Thanks to this article I can learn more. Expand my knowledge and abilities. Actually the article is very real EZPassCT login

That issue is in, I'm not really sure how this is node's responsibility given it depends entirely on the admin theme? Personally I find that the default content view in Claro has very little space taken up by the filters.

I'm not even sure if this is something we could fix? If you don't click Upload, how do we save that state?

This is still relevant, surprisingly the "You have not created any content types yet" message is hardcoded in the node-add-list twig template which is duplicated across 4 themes in core.

IMO this would ideally be in NodeController::addPage but that's probably going to be harder to get in.

Instead we can do something like this in the controller:

  if (count($types) > 0 && count($content) === 0 && $user->hasPermission('administer content types')) {
    // Return a hardcoded message, including cacheable metadata for permissions and the node_type list tag.
  }

We need a title and IS update here about exactly what we're proposing to change.

Still seems relevant.

Rolled into an MR, re #29 these services were marked as private so I don't think we need to do a deprecation dance here?

Can put back into review for more eyes but before RTBC summary will have to match

No worries. So might help to look at the reference issue this was created from.

Not a layout builder expert to say what’s needed or not but the summary needs to match the MR. Helps committers quickly understand a ticket

If it’s determined nothing is needed then the summary could include your findings and just a simple solution of remove the todo

Hi @smustgrave,

Apologies if I might be misunderstanding something here, but I just want to clarify the intent behind this change to ensure I'm following the best approach. I'm still getting started with contributing to Drupal, and I really appreciate your guidance.

The issue summary suggests that handleEntityDelete() should be updated to use isLayoutCompatibleEntity(), but after reviewing the implementation, I noticed that removeByLayoutEntity($entity) does not fail if the entity is not layout-compatible. Instead, it simply attempts to clean up inline_block_usage, setting layout_entity_id and layout_entity_type to NULL if applicable. If no matching records exist, nothing happens, which makes me wonder if the extra check is truly necessary.

I just want to make sure I'm not missing something here:

• Was there a specific issue that required adding isLayoutCompatibleEntity()?
• Is there any scenario where calling removeByLayoutEntity() directly could cause unintended side effects?

If the check is needed for a reason I’m not seeing, I’ll be happy to update it accordingly. Otherwise, removing it might help simplify the logic while still ensuring the cleanup happens.

Again, thanks for your patience and feedback—I really appreciate learning from this process!

writeGrants and node_access_write_grants are now gone.

The acquireGrants seem to make sense, so closing this as outdated. Please feel free to reopen and rescope the issue if you disagree.

Manually rolled into an MR

This definitely should be solved but as per #10 this seems baked into the node grants system?

Is this still something we want to consider? Our generic revision UI also sorts by revision id.

Is this still something we want to do? As per #4 it seems like functionality that might be used, I'm struggling to find examples in gitlab though.

The hooks are fired from the node_access_grants function, that function is called in several places with a hardcoded view operation, but NodeAccesControlHandler does pass in the $operation to NodeGrantDatabaseStorage::access which is passed to node_access_grants.

So reading the issue summary and title

update InlineBlockEntityOperations::handleEntityDelete() to use isLayoutCompatibleEntity()

Still seems to be needed.