Problem/Motivation
When you disable a text format it is not possible to re-enable it. This seems counter to the behavior in other situations, such as Views, which allows disabling than re-enabling a display. Furthermore, it can lead to major problems on a site if a text format was mistakenly disabled.
Steps to reproduce
Disable a text format and experience the disappointment upon realizing it can't be re-enabled. You'd need to create a new text format (yet can't create one with the same machine name), then change all items previously using the old one to use the new one.
Proposed resolution
Add an "Enable" operation for disabled text formats, which can be used to re-enable the disabled text format.
Remaining tasks
* Explore and implement an effective solution for ensuring cache reflects changes when a text format is re-enabled. This may involve adjusting or entirely rethinking the approach to cache tag management within preRenderText method of core /modules/filter/src/Element/ProcessedText.php #142 and #144
* Code review
User interface changes
The "Enable" operation is available to disabled text formats
API changes
Data model changes
Release notes snippet
Beta phase evaluation
Issue category | Bug because of unexpected behaviour. Upon disabling the text format it disappears from the form which makes it seem deleted, but it isn't actually deleted because you can't add a new text format with the same machine name. |
---|---|
Issue priority | Major because as cilefen states in #19:
|
Prioritized changes | The main goal of this issue is two-fold, fixing a DrupalWTF bug and improving usability. |
Disruption | Non-disruptive because only the filter format admin interface is changed. No API changes |
Comment | File | Size | Author |
---|---|---|---|
#143 | 2502637-test-that-fails-without-naaasty-cache-clear-WILL_FAIL.patch | 12.65 KB | bnjmnm |
#125 | Screenshot 2023-12-06 at 1.54.49 PM.png | 915.91 KB | bnjmnm |
#125 | enabled-disabled-text-format.png | 184.91 KB | bnjmnm |
#115 | diff-82-110.txt | 768 bytes | quietone |
#112 | 2502637-afterpatch1.png | 115.91 KB | gaurav-mathur |
Issue fork drupal-2502637
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
vegantriathleteComment #2
cilefen CreditAttribution: cilefen commentedComment #3
cilefen CreditAttribution: cilefen commentedComment #4
cilefen CreditAttribution: cilefen commentedComment #5
shumer CreditAttribution: shumer as a volunteer commentedThis patch change label and text for more reliable "Delete" instead "Disable".
Comment #6
shumer CreditAttribution: shumer as a volunteer commentedComment #8
shumer CreditAttribution: shumer as a volunteer commentedErrors fixed.
Comment #9
legolasboIsn't there a link template 'delete' which can be used?
The path should also be /delete
Comment #10
vegantriathleteIt's NOT actually deleting the text format. I don't think it's a good call to rename it Delete. That would imply that the text format could be created again, which is not the case. If you try to create the text format again you will get an error along the lines that it already exists.
If the text is going to be change to Delete, then the format should actually be deleted.
If the text is going to stay Disable, then the ability to re-enable it should be added.
Comment #11
legolasbo@vegantriathlete,
I only reviewed the code, not the functionality even though i guessed as much. Which is why i hinted at using an actual delete link.
Comment #12
cilefen CreditAttribution: cilefen commentedIs that an accurate title?
Comment #13
cilefen CreditAttribution: cilefen commentedComment #14
shumer CreditAttribution: shumer as a volunteer commentedOk, let's try one more time )
Now disabled format will appear in the formats list, with warning message.
Comment #15
legolasboComment #16
legolasboThe screenshot below shows the confirmation page. It's text is no longer correct.
The screenshot below shows the "Text formats and editors" page after disabling a text format. There is no way for the user to re-enable a text format, nor is it possible to create a new format with the same name. We should add an action button to enable the text-format once it has been disabled.
This format has been disabled and is no longer available.
Any content stored with this format will not be displayed.
Comment #17
shumer CreditAttribution: shumer as a volunteer commentedEnable action added.
Comment #18
cilefen CreditAttribution: cilefen commented#17 seems to work in manual testing. I am tagging "Needs usability review" so a specialist can chime in.
Comment #19
cilefen CreditAttribution: cilefen commentedActually, there is no workaround for GUI users (imagine "I want my text format back!"), so this is major.
Comment #20
shumer CreditAttribution: shumer as a volunteer commentedI hope it will pass review )
Comment #21
cilefen CreditAttribution: cilefen commentedThe status quo is the same behavior in Drupal 7. But, why disallow the re-enabling of a text format?
Comment #22
legolasboenabling?
That seems obvious to me, I think this line should explain the result of that action. i.e. Any content stored with this format will be displayed again.
Also, this needs tests
Comment #23
shumer CreditAttribution: shumer as a volunteer commentedText fixed and test for format enable operation added.
Comment #25
shumer CreditAttribution: shumer as a volunteer commentedLooks like writing tests it's not my best feature. Just adding patch with fixed text.
Comment #26
legolasbo@shumer,
Reviewing your changes would be a lot easier for me if you would provide an iterdiff. As for the tests, I can probably take a look at those tomorrow.
Comment #27
shumer CreditAttribution: shumer as a volunteer commented@legolasbo, interdiff attached
Comment #28
legolasboAttached patch adds tests and some small textual changes.
Comment #29
cilefen CreditAttribution: cilefen commentedThis needs a beta evaluation or it should be pushed back to 8.1.x. I am thinking we should push it back because there is so much going on right now and HEAD is the same as the Drupal 7 behavior.
Comment #30
legolasboAdded beta phase evaluation
Comment #31
legolasboComment #32
legolasboComment #33
Mark LaCroix CreditAttribution: Mark LaCroix commentedI thought I'd chime in here to say that while I appreciate the impulse to push a solution for this to 8.1.x, it brought up another problem for a simple single-user site I'm building which feels like a showstopper:
The warning message when disabling a text format says "any content stored with that format will not be displayed." However, this is wrong (at least in my case), as content using a format I disabled is still live on the site (localhost), even after clearing the cache. However, what's odd is I am now prevented from editing any nodes that had used that format.
When I try to edit one of these nodes, I get the dreaded "The website encountered an unexpected error. Please try again later." message. I can just delete the nodes, of course, and a user shouldn't be too surprised to lose field content if they delete a text format, but for many users struck by this, content in other fields in affected nodes might be very important (assuming this issue is not just limited to me, which is possible).
The warning message says "this cannot be undone," which I assumed meant was that I was actually deleting the format, and could recreate it using the same machine name, but as pointed out, this is impossible, and leaves cruft in the database.
The attempt to explain what is actually happening in the warning text is a strange thing because what it actually does is reinforce Drupal's notorious reputation for obtuse helper text, so even if it were accurate, it's easy to ignore it's intended meaning.
At the very least, it should be an "expected" error, not an "unexpected" one, and if it's not just me, indicates that this is a bigger issue than already recognized.
Comment #34
legolasbo@Mark LaCroix,
We can still address this in 8.0.x as per the beta phase evaluation. If you could review the code that would be another step in the right direction.
Can you provide steps to reproduce the unexpected error? If you can, please open a new issue for it and reference it here. What you are describing sounds like something that's major or even critical.
Comment #35
Mark LaCroix CreditAttribution: Mark LaCroix commentedDone and done! Thanks for the clue:
https://www.drupal.org/node/2555973
Comment #36
SGhosh CreditAttribution: SGhosh commentedPatch updated wrt latest code in 8.0.x branch.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThis looks like a design choice. What would have been the reason?
Comment #38
legolasbo@pjonckiere,
That code was added in #1987124: Convert filter_admin_format_page() and filter_admin_overview() to a Controller, when the callbacks were converted to a controller. The issue we are currently working on was actually discovered by dawehner while reviewing that patch. That method was added to keep the functionality the same as in D7. This behaviour is really strange however and should also be fixed in D7 in my opinion, which warrants it's removal.
Comment #39
yoroy CreditAttribution: yoroy as a volunteer commentedAdding two related issues from the D7 era that may shed some light on why things currently work as they do. This is a complex topic :-)
Comment #40
swentel CreditAttribution: swentel commentedComment #41
XanoRe-roll.
Comment #42
XanoThe enable and disable routes need CSRF tokens, because they respond to GET requests, but change the site's state.
Why was this added? We do not prevent any entities of other types from being edited when they are disabled, and the parent method already hides the disable operation for already disabled formats.
Comment #44
legolasboPatch no longer appies, rerolling and fixing tests.
Comment #45
legolasboRerolled and fixed tests. Also made a small interface text change.
Comment #46
legolasboAttributed organisation.
Comment #48
legolasboAlso fixed the remaining tests.
Comment #49
legolasboOops
Comment #50
Bojhan CreditAttribution: Bojhan as a volunteer commentedNothing to review, sounds like a straight bug.
Comment #51
cilefen CreditAttribution: cilefen commentedI think #45 is the latest patch and this is the reroll.
Comment #53
cilefen CreditAttribution: cilefen commentedI am ignorant of how to handle CSRF in WebTests. Does anyone know how that is worked-around?
Comment #55
cilefen CreditAttribution: cilefen commentedI was overthinking it.
Comment #56
cilefen CreditAttribution: cilefen commentedComment #59
cilefen CreditAttribution: cilefen commentedTests should pass now.
Comment #60
Pepe Roni CreditAttribution: Pepe Roni as a volunteer commentedIt is possible to reenable disabled text formats with the configuration manager:
Voilá, the text format is available again. This is working but not user friendly :(
PS. It may be that I have not used the correct English labels; I'm currently working with German translations.
Comment #61
cilefen CreditAttribution: cilefen commentedWe should backport this.
Comment #62
dqdTBH, I am not sure if I've tracked that issue correctly because it looks like it has been gone off course a little bit. To come back to the OP text:
Exactly. This is an obvious UI logic caveat against common UX standarts regarding the common understanding of visually "disabling" vs. "deactivating or deleting" something.
Since (like reported) the format is indeed NOT deleted and still there but disabled, with unxpected side effects like being hidden, disabling content which is using it, being unable to enable at the same place etc. We can talk about multiple issues here:
Respecting all issues and side effects not obvious for all from the first look the selection dropdown should include:
And only the delete option should make this text format disappear from the list of text formats if we want to stay in common practice of UI concepts and UX.
Comment #63
cilefen CreditAttribution: cilefen commented@diqidoq I agree with most of what you wrote. We should do one of the following:
Comment #64
caten8 CreditAttribution: caten8 commentedThanks to Pepe Roni, #60 works.
By the way, if you have any text in those fields and they disappeared after disabling your text formats, don't panic. Just run update.php after re-enabling them and your stored content will return.
Also, if you'd rather make changes to the database, under D8, text format configurations are now in the "config" table.
Run this SQL Query:
SELECT `collection`, name FROM `config` WHERE name LIKE 'filter.format%';
and you'll find all four default text formats listed.
Your data is stored in a BLOB field. You can download it to examine what's inside. If you're comfortable with db queries you can run an UPDATE to change status from 0 to 1 but be careful with SET commands, they're powerful and can overwrite other data unless you know what you're doing. You can also expose BLOB contents by changing settings in your phpMyAdmin config file. Here's a helpful post to get you started:
https://groups.drupal.org/node/134269
Comment #66
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #67
tkoleary CreditAttribution: tkoleary at Acquia commentedNeeds a new patch to reflect the comments in #62
Comment #71
Manav CreditAttribution: Manav as a volunteer commentedCurrently working with 8.3.x and faced the same issue so I have done all this by code.
I know its not a good solution but its works for me.
Comment #74
MartinMa CreditAttribution: MartinMa commentedThere is just another problem: I get a lot of messages in dblog if I "disable" a text format:
Really boring ...
Comment #76
MartinMa CreditAttribution: MartinMa commentedHugh, problem still not solved?
Just forgott the problem and deleted text format basic_html again and had problem again.
Content of pages with Text format are not shown. Some admin pages make fatal error. No error messages in dblog via web interface but via drush. And a lot of other strange behaviour.
Fortunately I have a new back.
Is it so impossible to prevent the default text formats to be deleted?
Or to put meanwhile a warning message to the admin page of text formats?
Comment #77
MartinMa CreditAttribution: MartinMa commentedUpgraded to critical. Because deleting default text format can cause a lot of problems - whole content of body texts of website not shown - if you dont have a new backup!
Comment #78
Sylvebois CreditAttribution: Sylvebois commentedIn reply to previous post (#77), there's a trick to get the disabled text format back :
Comment #80
sano CreditAttribution: sano commentedDrupal 7 users can re-enable the disabled formats by setting status to 1 in the filter_format table for the disabled file formats.
Comment #81
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedRe-rolled the patch.
Comment #82
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #83
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #84
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedVerified and test by applying the patch #82.It is working fine and looks good.
Pre-requisite -
Clear the cache of the site after applying the patch once.
Steps to test -
1. Go to the admin site.
2. Go to the config/content/formats.
3. Click on icon against the configure.
4. Click on disable.
5. Text format gets disable.
6. You will see the enable button.
7. On clicking on enable , we will get a pop-up and again click on enable.
8. You can disable and enable the text format field anytime.
Comment #85
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #86
alexpottThe needs so automated tests to prove it works.
Also there are a number of other complications here. That kind of block or at least need to go long side this work:
Comment #87
alexpottAlso note that this issue feels like a duplicate of #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) which has some relevant security discussion and UI discussion. It feels like there really should only be one issue. #914458: Remove the format delete reassignment "feature" Has some of the security discussion.
Reading those issues it seems that it is seems re-enabling a text format is considered ok from a security perspective. The problem is deleting a text format. However the other issue points out that one of the features of disabling a text format is that it is then hidden in the UI. So perhaps instead of having disabled formats mixed in the with enabled formats we should have a separate page that lists them instead.
It's a shame that this issue was not declared a duplicate of #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) before there was so much progress here. Now it's exceptionally hard to know what to do.
Comment #88
alexpottI guess one way of the duplicate issue is for this issue to be scoped back to the issue title which is
. So this issue could make the list visible and #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) could deal with providing a re-enable.
Comment #89
thursday_bw CreditAttribution: thursday_bw as a volunteer commentedI vote that this issue be closed as a duplicate.
The work that's been done here is worth while, so also copy the latest working patch (#82) from here to that patch.
Obviously cross reference both patches.
Let this work live on in the other issue, and this one can worry us no more, and reduce issue queue clutter.
Comment #90
thursday_bw CreditAttribution: thursday_bw as a volunteer commentedWould be worth at the same time, perhaps making reference on the other ticket of people who have contributed this to this issue.
To ensure when that ticket is evenuntually fixed, due credit is issued.
Comment #99
pameeela CreditAttribution: pameeela commentedI know that typically the newer issue should be closed as a duplicate but in this case there is much more recent work in this ticket, so I'm closing #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled) instead. I've moved credit from that issue also.
Comment #100
fkelly12054@gmail.com CreditAttribution: fkelly12054@gmail.com as a volunteer commentedI stumbled into this and related threads when either I improperly installed or encountered a bug in a contrib module with the result that I could not save changes to the full_html text format. After some efforts I wound up disabling full html then could not view any content on my site ... or rather the content was badly mangled.
Somehow I managed to get full_html restored and my site working. But experimenting with a test version and looking at the database it appears that, even if you disable the text format, ignoring the warning emitted: while the text format is gone from the admin/config/content/formats/add screen and can't be added back because it is seen as a duplicate, the text format remains in the text format field of tables such as node_body. So you've got a bunch of content items which depend on a text format that doesn't exist in your system.
I should say that it doesn't exist operationally but it exists "enough" to keep you from re-enabling it.
Perhaps the fix should include not allowing any text format to be disabled that has content items using it? Re-enabling a disabled text format might be useful too, but the real hole that's dug here is disabling text formats that content items depend on in the first place.
Comment #101
Joachim NamysloI'd just like to mention; currently this behaviour makes the minimal installation profile useless. Just because noone mentioned anywhere that many modules reley on a text format called full_html. So if you do not create it or disable it in accident the installation is broken. Perhaps minimal should install all standard text fromats to avoid that bug. Maybe this is somewhat for another issue.
Comment #102
dqdSince there is more and more understanding in the community about that text formats can not and should not be removed but rather disabled only and be re-enablable (all for good reasons) we should discuss somewhere else asap if we should still allow modules to install their text formats without warnings, since these modules aren't able to uninstall no more forever.
Comment #104
stephencamilo CreditAttribution: stephencamilo as a volunteer commentedComment #105
rkollerComment #107
zenimagine CreditAttribution: zenimagine commentedI created a new text format, which is no longer used on my website. I replaced it in all my posts.
How can I permanently delete it ? I don't want to disable it, I want to delete it.
It is no longer used and it is totally useless.
Why don't you leave the choice to the administrators ?
This thing is incomprehensible. Another weird Drupal thing. No logic there.
Comment #108
cilefen CreditAttribution: cilefen commented@zenimagine
This is an open, critical bug. Comments like the one you wrote are unhelpful. Instead, contribute to the effort to fix this.
Comment #110
DanielVezaIt would be good to move this issue forward since it's marked as a critical, so I've rolled the patch against 10.1, changed the text to be more clear and attempted to address the feedback in #86.
I've also added a test for the enabling and disabling of the filters. We could probably expand this to test the formats are correctly enabled when doing through the enable form in the UI.
Comment #111
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedHi i will review this patch.
Comment #112
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedHi i applied patch#110. it is work fine and looks good.
Steps to test -
1. Go to the admin site/config/content/formats.
2. Click on icon against the configure.
3. Click on disable.
4. Text format gets disable.
Without apply patch there no option to enable.
After apply patch
1. You will see the enable button.
2. On clicking on enable.
3. we will get a pop-up and again click on enable.
4. You can disable and enable the text format field anytime.
adding screenshot for the reference.
Thank You
Comment #113
rkollerThanks for the reroll and working on the issue! I've also tested the patch in #110 and it applies to 10.1.x-dev. Before applying the patch I've disabled a newly created text format. After applying the patch the text format that disappeared on disable was visible again with the default option
Enable
on the operations button. So the problem from the issue summary is fixed.The only detail I wonder about there is no visual cue to distinguish the state a text format is in aside reading the operations button label (and there is also the slightly smaller button width due to the shorter character count of the
Enable
compared to theConfigure
label). For theViews
overview page (/admin/structure/views
), a content typesmanage form display
page (eg/admin/structure/types/manage/article/form-display
), and a content typesmanage display
pages (eg/admin/structure/types/manage/article/display
) you have dedicatedenabled
anddisabled
sections.Manage form display
andmanage display
have the same draggable item pattern like thetext formats and editors
page - in contrast on theViews
page the items aren't draggable. I think it would be a good idea for thetext formats and editors
page to keep that interface pattern consistent with the aforementioned pages. But I am not sure if it would make sense to solve it within this issue or if creating a follow up issue would be the better choice? With a follow up the existing critical issue of a disappearing text format wouldn't be delayed any further.Comment #114
Rajeshreeputra#110 patch works perfectly fine, hence moving ahead.
Comment #115
quietone CreditAttribution: quietone at PreviousNext commented@DanielVeza, thanks for the reroll. Remember to add an interdiff so the reroll can be reviewed.
The Issue Summary on this issue needs to be completed. A description of the proposed resolution is needed so the reviewer/committer knows what is expected. I am adding the tag and updating the remaining tasks.
This is making UX changes and should have a UX review, I am adding the tag.
The patch was rerolled and changes made to address #86. The reroll and those changes have not been reviewed.
Because this is critical, I am adding the interdiff.
A reminder that a patch must pass the core gates before it can be RTBC.
Comment #116
rkollerUsability review
We've discussed this issue at #3360113: Drupal Usability Meeting 2023-05-19. That issue will have a link to the recording of the meeting.
For the record, the attendees at today’s usability meeting were @aaronmchale, @benjifisher, @blackbamboo, @rkoller, and @simohell.
There were only two details noted during the discussion:
1. There was a consensus to clarify the implications what the sentence is communicating that is used on the confirmation page for disabling a text format. The current version of the sentence is:
Any content stored with the %format format will not be displayed.
The recommended change is:
Any content saved with the %format text format will not be displayed on the site until it is resaved with an enabled text format.
The new version clarifies that content which was saved with the particular text format won't be displayed on the frontend to any visitor until that content is resaved with an enabled text format. The change could already be done in this issue or moved to a followup issue.
2. In regards of the visual representation of the list of enabled and disabled text formats there was an agreement that the current state has downsides making it challenging to distinguish between enabled and disabled text formats. Currently there are no visual cues helping the user to clearly distinguish in between enabled and disabled text formats - you just have grey buttons so you have to read each of them.
one option how to tackle this is the suggestion brought up in #113. The state (enabled/disabled) would be clearly visible and it would be consistent with the pattern used for example on the Views-page. But the downside to this approach, in contrast to Views a site usually doesn't have that many text formats, plus in case no text format is disabled, there would be an empty disabled table at the bottom of the page. An alternativ option that was brought up instead was adding a column containing the information if a text format is enabled or disabled. The group agreed moving the discussion how the enabled/disabled state is communicated to a follow-up issue with resolution TBD.
Aside those two details the patch is in excellent shape with no complaints. I'll remove the needs usability review tag.
Comment #118
AlfTheCat CreditAttribution: AlfTheCat commented#110 did the job! Broken site is working again. Did not see that coming when I clicked "disable" :)
Thanks very much for the hard work on this long standing issue.
Comment #119
ressa CreditAttribution: ressa at Ardea commentedThanks everyone for working on fixing this critical issue. I just got hit by it (see #3271741-15: Module doesn't delete own config after uninstall) and it would be awesome to move it forward.
Comment #120
lauriiiI agree that we can open a follow-up for addressing #116.2. 👍 This is still a net win even if the UX isn't 100% perfect.
#116.1 seems like a great suggestion so let's address that here, since we're changing the text anyway.
Comment #123
omkar.podey CreditAttribution: omkar.podey at Acquia commentedFollow up #3406211: Improve representation of the list of enabled and disabled text formats for [#116.2]
Comment #125
bnjmnm@omkar.podey created followup #3406211: Improve representation of the list of enabled and disabled text formats so I'm removing that tag.
I updated the issue summary, so I'm removing that tag.
Concern
I manually created content with a text format, then disabled that text format. I am still able to access the content created with that text format despite what the confirmation warnings say. Perhaps this is because it's a full html format I tested? Will test further.I did get it so content using a disabled text format is not visible. It required a few extra steps - Disabling a text format does not seem to clear all caches that might be impacted by this. That is out of scope for this issue, and it's probably best if the warning remains as-is and makes the user aware of what will ultimately happen, even if it's not immediate due to caching.👇 Look at this page content created with a disabled text format. It is still being displayed despite the confirmation page warning stating otherwise
This needs additional tests
The only test added
testEnableAndDisableOfFilter()
successfully confirms enable/disable operation links appear as expected, but we also need to test and confirmComment #126
Wim LeersThis nerdsniped me 🤓
config:filter.format.full_html
should be invalidated when disabling.config:filter.format.full_html
should be associated with that output.I'm not seeing that? I'm seeing that the node is still rendered, but the field of that node (which contains text processed by the now disabled text format) should not be rendered. And that works in a manual test I just did locally?
\Drupal\Tests\filter\Functional\FilterAdminTest::testDisabledFormat()
already tests this?Comment #127
bnjmnmThanks for the extra context @Wim Leers -
That existing test confirmed that disabling a filter would result in the field being unavailable, but there wasn't anything for the use case of re-enabling the format in the UI and confirming it was again visible. I expanded the test in the MR to cover this scenario so everything I mentioned in #125 is now addressed.
Comment #128
smustgrave CreditAttribution: smustgrave at Mobomo commentedOh man if this gets fixed it would be huge! Should there be a follow up for deleting the format? Seems out of scope of this.
Only moving to NW as there appears to be a test failure.
Haven't tested yet.
Comment #129
omkar.podey CreditAttribution: omkar.podey at Acquia commentedAlso wrote a test, sorry couldn't push earlier but it's looking good already 👍, thanks.
Comment #130
bnjmnmThe test I added demonstrated that different caches need to be considered when re-enabling a text format, so I'm glad the test was there. I addressed this by triggering a pretty heavy cache clear when calling
enable()
on a text format. Maybe there's a way for it to be less broad despite the difficulty targeting something that formerly used a format. I'd like to think this is reasonably safe since re-enabling is probably an uncommon occurrence, most likely done in a non-production environment, and it's not unheard of to lose some cache when content-impacting config changes.Comment #131
smustgrave CreditAttribution: smustgrave at Mobomo commentedWould this be worth a follow up ticket but the ability to configure a disabled format? Have definitely hit the situation before where I wanted to uninstall some ckeditor module and a disabled format had it as a dependency. If so I'll happily open it.
But testing the MR as is
Disabled basic HTML and verified I can still see it in the list.
Went to create a node it's not there, good.
Went back and enabled basic HTML, it shows as enabled again.
Went to create a node and it's there again.
This looks good to me and even though some follow ups may be needed I think this helps most cases right now.
Comment #132
mglamanI don't think so. That would be fixed by another issue #2579743: Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed or #3056633: It is not possible to uninstall a module that provides a filter plugin - Drupal\Component\Plugin\Exception\PluginNotFoundException: The "{filter}" plugin does not exist
Comment #133
Wim Leers#132 is right, that is a different problem.
Comment #134
quietone CreditAttribution: quietone at PreviousNext commentedI'm triaging RTBC issues.
It is always a pleasure to work on older issues.
I read the Issue Summary and scrolled down seeing that there was a Usability review and reviews by core committers. I then went to read the MR. I left suggestions for comments as well as an item for adding return type hints. I spent most of my time, so far, on reading the test. The fact that there are two filters, 'Enabled filter' and 'Disabled filter' and that the 'Enabled filter' is disabled is just confusing. I think the names need to change so that are not indicating the state of the filter, which is changing in the test. Adding detail to the comments would help as well. That will go a long way to help others working on the test in the future. I am also not sure why two test filters are needed.
Therefore I am setting to NW.
Comment #135
quietone CreditAttribution: quietone at PreviousNext commentedUpdating credit.
Comment #136
smustgrave CreditAttribution: smustgrave at Mobomo commentedNot super thrilled with the solution I did to check the edit (configure) links. But using linksHrefExists was giving false positives.
Comment #137
dwwThanks for all the work on this so far! Let's smash this very old, critical bug.
Opened a handful of MR threads. A few of them serious, mostly very nit-picky suggestions. But since @smustgrave is working on the code, I'll only make MR suggestions, not push commits, so I can be eligible to RTBC this...
Thanks again,
-Derek
Comment #138
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #139
dwwMostly looks great, thanks! Left a few more comments on the MR. Most of the threads could be closed, but a few still need another look before this is RTBC.
p.s. Crediting smustgrave for working on the MR, and quietone and myself for reviews. Haven't thoroughly reviewed the issue for any other credit updates, but wanted to get a little closer.
Comment #140
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed additional feedback.
Comment #141
dwwThanks for removing the new admin user and the extra drupalLogin(). I think that’s best.
I have no other concerns. All the MR threads have been addressed. Title and summary are accurate. Pipelines are green. All is well. RTBC!
Thanks again,
-Derek
Comment #142
Wim LeersAFAICT the cache invalidation logic in
::enable()
is not correct nor necessary? 😅I could totally be missing something though. 😇 But in that case, docs are needed to explain why this additional cache invalidation logic is needed. 🙏
Comment #143
bnjmnmAttached is a patch with the test that fails if I don't do that cache clear in
enable()
. I couldn't find a reliable tag to invalidate. When a text format was disabled, the entity would no longer have the correspondingconfig:filter.format.the_text_format
tag. The scorched earth cache bin blowup was how I could get it to work, but I'd prefer a more targeted solution as well. Clearly there is data in the DB that is aware of formerly-used text formats, and perhaps that is the way to do it without annihilating entire bins?Comment #144
dwwI queued the patch in #143 for testing, and lo, it fails as expected:
That's after enabling the filter, and no other cache shenanigans. The body field doesn't re-appear, since we didn't invalidate the caches.
There very well might be a cleaner, more targetted way to do this, but we need to do something. Back to Wim for a 2nd opinion.
Thanks!
-Derek
Comment #145
smustgrave CreditAttribution: smustgrave at Mobomo commented@Wim Leers wonder if you've had a change to take another look?
Comment #146
smustgrave CreditAttribution: smustgrave at Mobomo commentedGoing to go ahead and mark it to keep the issue from stalling. Re-tested and still seems to behave correctly.
@Wim un-assigning but please feel free to review or change status :)
Comment #147
quietone CreditAttribution: quietone at PreviousNext commentedThere is a suggested change in the MR that has not been addressed. There is a request for documentation in #142 and a question in #144. Also, there are files here that need to be hidden. Since there are so many I have done a few. Setting to NW.
I have not reviewed the MR or tested the change.
There is some misunderstanding here. Issues stall for many reasons (difficulty of problem, family, work etc) but the fix is not to set it to RTBC. In fact, that can have the opposite effect. Issues that are RTBC are often not looked at by non-committers, so it sits. Then when a committer does look at it they see the unfinished work and make a comment. And worse, this takes committer time away from issues that are truly ready for commit. It would be better to make sure that the Issue Summary is up to date, that the remaining tasks are clear so everyone knows what needs to be done and keep the issue at Needs Work or Needs Review as needed.
Comment #149
Wim Leers#143: that should not be possible 😬
So I stepped through it with a debugger. And sure enough, the cache tags for
were:
node:1 node_view rendered user:2 user_view
… but
\Drupal\filter\Element\ProcessedText::preRenderText()
should have bubbled the cache tags:So is there some special handling for disabled text formats that explains this? YES THERE IS! In that same method:
👆 This should do the same cache tag merging logic
if ($format !== NULL)
.Comment #150
Wim LeersDone.
Comment #151
Wim LeersComment #153
smustgrave CreditAttribution: smustgrave commentedAppears to be 1 more thread open from @Wim Leers around core/modules/filter/src/FilterFormatListBuilder.php
Comment #154
thursday_bw CreditAttribution: thursday_bw as a volunteer commentedHey everyone, I've updated the "Remaining Tasks" section of the issue description to provide clearer summaries and direct links to comments for better context. This should help streamline our focus and make it easier for contributors to understand and tackle the tasks at hand.
On another note, there's a mention about an unresolved aspect related to
core/modules/filter/src/FilterFormatListBuilder.php
attributed to @Wim-Leers.I'm having trouble finding the original discussion. Does anyone have the scoop on this? Would be great to clear that up so we're all on the same page.I found it https://git.drupalcode.org/project/drupal/-/merge_requests/5685#note_268326