- 🇫🇷France DrDam
@norman.lol :
In which class do you propose adding this method?
- 🇩🇪Germany geek-merlin Freiburg, Germany
Since TrustedCallbacks → , sth like this does not work anymore:
$element['#pre_render'][] = [HelpCallbacks::class, 'preRenderHelp'];
Fix is:
$element['#pre_render'][] = HelpCallbacks::class . '::preRenderHelp';
- 🇮🇳India mohit_aghera Rajkot
mohit_aghera → changed the visibility of the branch 11.x to hidden.
- 🇮🇳India mohit_aghera Rajkot
mohit_aghera → changed the visibility of the branch 2637808-languageformatter to hidden.
- 🇮🇳India adwivedi008
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 - 🇺🇸United States dcam
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.
- 🇦🇺Australia acbramley
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. - First commit to issue fork.
- 🇦🇺Australia acbramley
acbramley → changed the visibility of the branch 11.x to hidden.
- 🇦🇺Australia acbramley
acbramley → changed the visibility of the branch 9.2.x to hidden.
- 🇺🇸United States rhovland Oregon
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.
- 🇺🇸United States rhovland Oregon
Why did you overwrite all of the changes made to this issue?
- 🇺🇸United States mradcliffe USA
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.
- 🇺🇸United States sker101 NYC
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.
- 🇺🇸United States jmev
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.
- 🇬🇧United Kingdom MrDaleSmith
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?
- 🇬🇧United Kingdom MrDaleSmith
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?
- 🇧🇷Brazil brandonlira
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!
- 🇦🇺Australia acbramley
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.
- 🇺🇸United States dcam
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.
- 🇧🇷Brazil brandonlira
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!
- @brandonlira opened merge request.
- First commit to issue fork.
- 🇦🇺Australia acbramley
If this is a bug, as per the IS, we need steps to reproduce.
- 🇫🇷France prudloff Lille
Failing AssetAggregationAcrossPagesTest seems unrelated.
- 🇺🇸United States smustgrave
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.
- 🇺🇸United States smustgrave
Don't know if I follow the task, if this still needed? Should it be moved out of PMNMI?
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Very nice to see we’ve come so far that we can deprecate that! 🥳
- 🇺🇸United States smustgrave
Since there hasn't been a follow up going to close out but if still an issue in D11 please reopen.
- 🇺🇸United States smustgrave
https://git.drupalcode.org/issue/drupal-2973515/-/jobs/4680214 shows the test coverage and the actual fix seems pretty straight forward to me.
- 🇬🇧United Kingdom MrDaleSmith
Change applies cleanly, test pass and the correct dependency is now added to the exported config. I think this is OK to go.
- 🇬🇧United Kingdom MrDaleSmith
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?
- 🇬🇧United Kingdom MrDaleSmith
Looks good to me, and the random test failure has been resolved by a re-run.
- @sivaji_ganesh_jojodae opened merge request.
- 🇮🇳India Sivaji_Ganesh_Jojodae Chennai
sivaji_ganesh_jojodae → changed the visibility of the branch 3514936-modulesuninstallform.php-assigns-string to hidden.
- 🇮🇳India KumudB Ahmedabad
Steps to Reproduce:
- Install a standard Drupal profile.
- Add a required field to the Tags taxonomy.
- Ensure that the Tags field on the Article content type is configured to auto-create new terms.
- Create a new Article with a tag that does not exist.
- The new term is created with the required field empty, leading to data inconsistency.
Here I have attached After fixing Screen shot
- @kumudb opened merge request.
- First commit to issue fork.
- 🇺🇸United States dcam
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.
- 🇦🇺Australia acbramley
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.
- 🇦🇺Australia acbramley
Rebased onto 11.x and made some minor changes. Updated IS and title to match solution.
- First commit to issue fork.
- @acbramley opened merge request.
- 🇦🇺Australia acbramley
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.
- 🇬🇧United Kingdom catch
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.
- 🇺🇸United States ashrafabed
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.
- @dcam opened merge request.
- First commit to issue fork.
- 🇧🇷Brazil charlliequadros
charlliequadros → changed the visibility of the branch 2873447-views-rest-export to hidden.
- 🇺🇸United States rhovland Oregon
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.
- 🇺🇸United States brad.bulger
I am applying the patch in #20 to this contrib version of tracker, it works without issue.
- heddn Nicaragua
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.
- 🇺🇸United States jmev
I've tested the patch by @rhovland on the checkout screen (Commerce 2.36) and it works well. Thank you!
- 🇺🇸United States smustgrave
Game up as a BSI target
After 7+ years wonder if still an issue with all the changes to views from 2018 to now?
- 🇺🇸United States smustgrave
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.
- 🇪🇸Spain manuel.adan 🌌
Duplicate of 🐛 Toolbar overlap content with big pipe enabled Needs work ?
- 🇪🇸Spain ssantaeugenia
Thanks for this great patch. I've attached the modified patch to also affect JavaScript.
- 🇦🇺Australia acbramley
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?
- 🇳🇿New Zealand quietone
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 .
- 🇦🇺Australia acbramley
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.
- 🇺🇸United States rhovland Oregon
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.
- @rhovland opened merge request.
- 🇺🇸United States rhovland Oregon
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 FixedThe 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).
- 🇪🇸Spain akalam
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)"
- @akalam opened merge request.
- First commit to issue fork.
Thanks to this article I can learn more. Expand my knowledge and abilities. Actually the article is very real EZPassCT login
- 🇦🇺Australia acbramley
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.
- 🇦🇺Australia acbramley
I'm not even sure if this is something we could fix? If you don't click Upload, how do we save that state?
- 🇦🇺Australia acbramley
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. }
- 🇦🇺Australia acbramley
We need a title and IS update here about exactly what we're proposing to change.
- @acbramley opened merge request.
- First commit to issue fork.
- 🇺🇸United States smustgrave
Can put back into review for more eyes but before RTBC summary will have to match
- 🇺🇸United States smustgrave
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
- 🇧🇷Brazil brandonlira
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 useisLayoutCompatibleEntity()
, but after reviewing the implementation, I noticed thatremoveByLayoutEntity($entity)
does not fail if the entity is not layout-compatible. Instead, it simply attempts to clean upinline_block_usage
, settinglayout_entity_id
andlayout_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 r
emoveByLayoutEntity()
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!
- Was there a specific issue that required adding
- 🇦🇺Australia acbramley
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.
- @acbramley opened merge request.
- First commit to issue fork.
- @acbramley opened merge request.
- First commit to issue fork.
- 🇦🇺Australia acbramley
Is this still something we want to consider? Our generic revision UI also sorts by revision id.
- 🇦🇺Australia acbramley
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. - 🇺🇸United States smustgrave
So reading the issue summary and title
update InlineBlockEntityOperations::handleEntityDelete() to use isLayoutCompatibleEntity()
Still seems to be needed.