- 🇺🇸United States nicxvan
Actually I want to make it less likely that a random test hook will collide with this and some more comments
- 🇺🇸United States smustgrave
idk if a good search but I looked for $definition['provider'] and didn't see any other spots that are doing translations.
- 🇺🇸United States kentr Durango, CO
Actually, #222 was also NW for the change record.
- 🇦🇺Australia mstrelan
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?
- 🇨🇦Canada mgifford Ottawa, Ontario
@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.
- @hswong3i opened merge request.
- 🇺🇸United States nicxvan
Closed one of the child issues as a duplicate and added the other issue.
- 🇺🇸United States xjm
Well, let's stick with "outdated" so that people don't go "what are they thinking?!".
- 🇺🇸United States xjm
I've never been so happy to see a terrible bug be closed without being fixed.
- 🇺🇸United States xjm
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.).
- 🇺🇸United States xjm
🐛 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.
- 🇺🇸United States xjm
Also saving credit for various reviewers and discussion participants.
- 🇺🇸United States xjm
Saving credits according to our core issue credit guidelines → . Thanks!
- 🇺🇸United States xjm
It might be a dupe of #2559313: Parse composer.json for the project name for update module → at this point, however.
- 🇺🇸United States xjm
Titling to describe the remaining scope of the problem. As far as I know this is still an issue these days.
- 🇺🇸United States xjm
Adjusting saved issue credits according to credit guidelines.
- 🇺🇸United States xjm
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:
- Let's address the deprecation stuff to get that out of the way.
- 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!
- 🇺🇸United States kentr Durango, CO
My last comment should have referenced #29. I edited my comment to correct.
Also adding STR to help with testing / reviewing.
- 🇺🇸United States nicxvan
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.)
- @mrinalini9 opened merge request.
- First commit to issue fork.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States xjm
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!
- 🇦🇺Australia acbramley
Back to PMNMI then since we still need steps to reproduce this.
- 🇺🇸United States nicxvan
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.
- 🇺🇸United States nicxvan
Needs a rebase, but I have a clean test that shows this is still an issue.
- @nicxvan opened merge request.
- 🇺🇸United States brad.bulger
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.
- 🇺🇸United States smustgrave
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.
- 🇩🇰Denmark ressa Copenhagen
Great idea about reducing the strain on the localization servers @heddn.
- 🇺🇸United States smustgrave
@acbramley for 🐛 Language attribute is not attached to node title on a full page view Active
- 🇺🇸United States smustgrave
Since there's been no follow up going to close out. If still a bug in D11 please re-open!
Thanks!
- 🇺🇸United States smustgrave
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!
- 🇸🇰Slovakia kaszarobert
On Drupal 10 and 11 none of the patches fix this, ModerationStateField's methods are not even called.
- 🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
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.
- @eelkeblok opened merge request.
- 🇮🇳India divyansh.gupta Jaipur
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. - @divyanshgupta opened merge request.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇫🇷France Nixou Toulon
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 ?
- 🇮🇳India divyansh.gupta Jaipur
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? - 🇺🇸United States smustgrave
If just a docs update believe what’s proposed in #8 makes sense
- 🇦🇺Australia mstrelan
For many of these we should probably autoconfigure the loggers instead Loggers can be autoconfigured for service classes implementing \Psr\Log\LoggerAwareInterface →
- 🇦🇺Australia mstrelan
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. - 🇦🇺Australia mstrelan
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.
- Install Drupal 11.x with standard profile, includes dynamic page cache and page cache modules
- Ensure render cache is not disabled in settings.php
- Visit the home page as an anonymous user, observe site name in the branding block
drush config-set system.site name "Test 1"
- Repeat step 3
- 🇺🇸United States xjm
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.
- 🇺🇸United States smustgrave
I dont mind marking but I also dont speak the language so maybe someone who speaks can review for accuracy?
- leymannx Berlin
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.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇳🇱Netherlands eelkeblok Netherlands 🇳🇱
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?
- @eelkeblok opened merge request.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
@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
- First commit to issue fork.
- First commit to issue fork.
- 🇬🇧United Kingdom oily Greater London
Test-only test now fails as it should. Other tests in the pipeline are breaking. Not sure if related.
- 🇧🇪Belgium keszthelyi Brussels
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.
- 🇺🇸United States smustgrave
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
- 🇬🇧United Kingdom oily Greater London
Fixed PHPCS. Rebased. Now several tests seem broken in pipeline.
- 🇺🇸United States smustgrave
Since there hasn't been a follow up I'm going to close out but am leaving all the credit assigned.
- First commit to issue fork.
- 🇬🇧United Kingdom catch
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!
- 🇬🇧United Kingdom joachim
> 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.
- 🇬🇧United Kingdom catch
::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!
- 🇮🇳India roshanibhangale
Hi,
I have tested this ticket on the Drupal 11.x, and working as expected.
Steps followed:
- Create a New Menu Type (/admin/structure/Menus/Add Menus), Save.
- Add the Menu to a Content Type (Structure > Content types > Basic page > Manage fields>Edit >Menu settings)
- Select the newly created menu from "Available menus", Save
- Delete the newly created Menu (Structure > Menus)
- Check if Config is Still There (Configuration > Development > Configuration Synchronization),
- Go to Export > Single item, Choose Content type, Select Basic page, Click Export
- 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.
- 🇺🇸United States xjm
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. :)
- 🇦🇺Australia acbramley
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!
- 🇦🇺Australia mstrelan
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.
- 🇺🇸United States xjm
(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.)
- @xjm opened merge request.
- 🇺🇸United States xjm
I almost made the branch name
3215368-щ
but then no one would have been able to type it. - 🇺🇸United States smustgrave
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?
- 🇫🇷France Nixou Toulon
I ended finally with this two patches (for drupal 10.4.5 and 11.2.2) for which the idea is the following :
- 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
- They neither can store which was their "original display in the duplication" and they do not really seem to need to know that
- 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.
- 🇫🇷France Nixou Toulon
After further investigation, I found the following :
- $exposed_form->preExecute() is call because $this->usesExposed() returned TRUE
- $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.
- 🇫🇷France Nixou Toulon
I reopen this issue as I encounter the same problem and was able to determine the reproduction steps.
Drupal 11.2.2 fresh install.
- Install Views Data Export
- Go into the Content view : admin/structure/views/view/content/edit/page_1
- Create a new page display and set it a path, then save
- Go back to the initial page display : admin/structure/views/view/content/edit/page_1
- Click on "Basic" near "Exposed Form"
- In the "For" list, choose "This page (override)". Apply, and save
- On the right in the select list "View Page", select "Duplicate as data export"
- 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).
- 🇺🇸United States dcam
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. - 🇬🇧United Kingdom catch
Thanks the check looks right but we should inject ModuleHandler here now.
-
larowlan →
committed 4a6a0bc3 on 11.x
Issue #360057 by kim.pepper, Akaoni, kbahey, smustgrave, gdd,...
-
larowlan →
committed 4a6a0bc3 on 11.x
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Committed to 11.x - congrats folks, this was the oldest open bug in Drupal core, AKA 'the one ⭕️' in #bugsmash parlance
Published change record.
- 🇬🇧United Kingdom catch
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.
- 🇺🇸United States dcam
I added the change record. I haven't addressed other feedback yet.
- 🇺🇸United States deasly
I updated the patch to be compatible with Core 11.2.2+. Patch applied cleanly on my install.
- 🇺🇸United States smustgrave
Since there's been no follow up going to close out. If still a bug in D11 please re-open.
- 🇺🇸United States smustgrave
Believe feedback has been addressed regarding system_requirements
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇳🇿New Zealand quietone
Made some changes to the CR, https://www.drupal.org/node/3525119 →
- 🇦🇺Australia kim.pepper 🏄♂️🇦🇺Sydney, Australia
Fixed conflict with 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active and added check for setting in
SystemRequirements::checkRequirements()
instead ofsystem_requirements()
. - 🇦🇺Australia acbramley
This is now fixing cacheability in both views and regular list builders.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Committed to 11.x and unpostponed the follow-up - thanks all
-
larowlan →
committed cbbda6cd on 11.x
Issue #2853054 by bbrala, borisson_, cilefen, jatingupta40, longwave:...
-
larowlan →
committed cbbda6cd on 11.x
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
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