- ๐ณ๐ฟNew Zealand quietone New Zealand
Regarding #157 and the idea to have a second page to list disabled formats I think this was answered in point 2 of the href=" https://www.drupal.org/project/drupal/issues/2502637#comment-15058495 ๐ Disabled text formats can't be seen in the GUI Needs work ">Usability report in comment #116. Specifically they state "The group agreed moving the discussion how the enabled/disabled state is communicated to a follow-up issue with resolution TBD."
- ๐ณ๐ฟNew Zealand quietone New Zealand
@vijayavelr, Welcome to Drupal! Thanks for the interest in this issue. Since your solution is a custom module and will never get committed to core I suggest you create a contributed module โ for that work. That is, if you wish to.
I also checked the priority and agree with alexpott that this is not Critical. There is not data loss and there is a work around. I have added the work around to the issue summary, so it is can be found.
- ๐ณ๐ฟNew Zealand quietone New Zealand
@vijayavelr, Welcome to Drupal! Thanks for the interest in this issue. Since your solution is a custom module and will never get committed to core I suggest you create a contributed module โ for that work. That is, if you wish to.
I was looking at other other configuration forms for examples of having a separate page for disable 'things'. There is none that I saw. The closest was search pages which simply add a column for 'Status'. That is easy to implement but does not fulfill the suggestion of a second page. For me, I think before coding and changes this should get direction from usability. I also am inclined to think that this should go in as an incremental step towards a better solution. I will ping in #usability.
I also checked the priority and agree with alexpott that this is not Critical. There is not data loss and there is a work around. I have added the work around to the issue summary, so it is can be found.
- ๐ฆ๐บAustralia swatigarg06
I tested patch #63 and it seems to be failing for 8.x-3.4.
- ๐ญ๐บHungary mxr576 Hungary
โฏ git rebase origin/11.x Auto-merging core/modules/jsonapi/tests/src/Functional/FileUploadTest.php Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php Auto-merging core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php Auto-merging core/modules/workspaces/tests/src/Functional/WorkspaceSwitcherTest.php CONFLICT (content): Merge conflict in core/modules/workspaces/tests/src/Functional/WorkspaceSwitcherTest.php Auto-merging core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php error: could not apply 0804e87ca2... Cherry-picked fix from https://git.drupalcode.org/project/drupal/-/merge_requests/8198 hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm <conflicted_files>", then run "git rebase --continue". hint: You can instead skip this commit: run "git rebase --skip". hint: To abort and get back to the state before "git rebase", run "git rebase --abort". Could not apply 0804e87ca2... Cherry-picked fix from https://git.drupalcode.org/project/drupal/-/merge_requests/8198
Added void return defs to tests again... PHPStorm could easily resolve that.
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.
- ๐ญ๐บHungary mxr576 Hungary
Yes, it is still relevant: https://github.com/drupal/core/blob/10.3.0-rc1/modules/book/src/Plugin/B...
- ๐ง๐ชBelgium BramDriesen Belgium ๐ง๐ช
RE #54 now it reads better to my mind ๐
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Should the manager just include that by default and this change would only be used for more exotic cases?
It could, but just like you I'd argue that that is out of scope. For now I would focus on fixing the bugs caused by not correctly setting any cacheable metadata when the applies() check returns FALSE.
- ๐บ๐ธUnited States neclimdul Houston, TX
left rtbc because this isn't a change this merge makes just something it highlights so could be a follow up.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
No worries :) I'm not a native speaker either but I tend to want to get the grammar right, so I tend to look these things up when in doubt. You did make me think, though, and I believe the simple present is preferred here, as the implementation is arguably still to be written at the time of reading the documentation.
Will change that little detail and leave at RTBC.
- ๐บ๐ธUnited States neclimdul Houston, TX
I think I understand the reason of the change and it makes sense. Previously you'd have to return true from applies if you _might_ apply and then builder would have to do the actual check and apply the cache-ability. Yeah, that's a pretty big gotcha when building these builders.
Looking at the merge, there's a lot of duplicate adding of the route to the cache-ability. I guess maybe this is a dumb question and probably well understood in the building of BreadcrumbManager but isn't varying by route kinda a baseline implied functionality for breadcrumb builders? That's why we pass the route_manager in right? Should the manager just include that by default and this change would only be used for more exotic cases?
If it helps the discussion, and example where it doesn't cache by route in core is PathBasedBreadcrumbBuilder. I found that's broken on our site and actually decorate the core version with one that adds the route to the cache-ability of the path breadcrumb builder.
- ๐ง๐ชBelgium BramDriesen Belgium ๐ง๐ช
Nah, itโs fine then ๐ again, not a native speaker. ๐
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Can't the "to" be removed?
That would make it broken English. You have the infinite "to add" and then "to" as you're adding to something (the CacheableMetadata).
Or specify to what we're adding metadata?
This documentation is for the $cacheable_metadata parameter, so I think it should be clear that that's the thing you're adding to?
Anything you specified here does not have to be repeated in the build() method as it will be merged in automatically.
You can use "Anything you specify here" or "Anything you specified here", depending on whether you want it to read like a forward-looking suggestion or vice versa. I wouldn't use "have", though. If you want we can change it to the present tense.
As for "merged in", it's because you merge whatever was specified into the final result. Because that sentence has no mention of "the final result", the "to" part of "into" is dropped. Example: John was about to walk into the river, as he walked in, he felt something sharp beneath his feet.
- ๐ง๐ชBelgium BramDriesen Belgium ๐ง๐ช
Already better ๐
The cacheable metadata to add to if your check varies by or depends on something
Can't the "to" be removed? Or specify to what we're adding metadata?
Anything you specified here does not have to be repeated in the build() method as it will be merged in automatically.
Anything you have? ... merged automatically?
I'm no native english speaker, but with above changes I suggested it would read easier to me.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Hopefully this is a bit easier on the eyes.
- ๐ง๐ชBelgium BramDriesen Belgium ๐ง๐ช
Maybe because I'm tired, but I had a really hard time reading the doc block to try and understand what it was trying to say. Grammatically strange sentences and missing capital letters as well.
Code wise not much to add to the review. Looks clean and the other comments are easy to read.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
@smustgrave Updated the IS to reflect the actual bug and how it should be fixed. The initial IS was just one case of running into it, but ๐ VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active exposes the problem in more detail as it also causes incorrect cache redirects to be written.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
Please don't convert MRs into patches as it creates a lot of unnecessary noise. Especially when the MR is still up for review and might change.
- ๐ณ๐ฑNetherlands mike.vindicate
Attached a patch that can be used for Drupal 10.2. Thanks for fixing the issue.
I have created a custom module patch disabled_text_formats_18062024.patch โ by listing the disabled formats with enable links
Step 1 :
Step 2 :
Step 3:
Step 4:
- ๐บ๐ธUnited States smustgrave
If still a valid task please reopen for 2.0.x of book contrib.
- ๐ง๐ชBelgium kristiaanvandeneynde Antwerp, Belgium
The MR is the correct approach, it's just that the issue summary needs severe updating.
The new approach is #10, #36 and #37In essence: Allow the applies() method to set cacheability. Because the breadcrumb builders run in the same order every time and because only one can build the actual breadcrumb, we don't need to care about ALL builder's cacheable metadata, just the data up until the builder that actually applies.
I'll try to update the IS tomorrow.
- ๐บ๐ธUnited States smustgrave
The issue summary mentions "Revert or fix #2699627: url.path cache context for breadcrumbs is unnecessarily granular โ " can I ask which one it is? Maybe this can be flushed out more. Don't think this is an API or model change so put those as N/A
- ๐ณ๐ฟNew Zealand quietone New Zealand
Just adding a link to the comment quoted in #157. It is at #949220-49: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) โ .