How to reproduce:
- Install site on Minimal installation profile.
- Enable Views module.
- Go to Recent log messages (admin/reports/dblog) page.
- Click Delete link for a clear log.
- Press Confirm on the next step (Are you sure you want to delete the recent logs? page).
- Update Recent log messages page.
And now we can see the following item in the log:
Missing text format: basic_html.
Notes on the patch
The patch does the following:
- Changes the empty text for the dblog view to not use a text format.
- Provides an update hook that updates this part of the view if it hasn't been customized, for existing sites, and a test for the update path.
- Removes test code in DbLogViewsTest that was working around this bug by creating the Basic HTML format before running the parent method. The parent method (which tests the no-views version of the log) is now testing that you can clear the log and see the empty text.
- Adds a new test that ensures the empty text in this view doesn't get updated later to use a text format.
Comment | File | Size | Author |
---|---|---|---|
#34 | 2916898-34.patch | 5.52 KB | jhodgdon |
#33 | interdiff-2916898-29-33.txt | 495 bytes | dagmar |
#33 | 2916898-33.patch | 5.52 KB | dagmar |
#29 | 2916898-29.patch | 5.52 KB | dagmar |
#29 | interdiff-2916898-27-29.txt | 644 bytes | dagmar |
Comments
Comment #2
lexhoukComment #3
dagmarThanks! Makes sense. With this change we could also get rid of the code of the method DbLogViewsTest::testDBLogAddAndClear()
Could you do that in your patch?
Comment #4
nicolas.rafaelli CreditAttribution: nicolas.rafaelli as a volunteer and at Globant commentedHere is the proposed change.
Comment #5
NickDickinsonWildeWorks as expected.
I do not believe it needs additional tests as the test working around the issue has been removed.
Comment #6
tstoecklerThis lead me to open #2917817: Views text area plugin does not declare a dependency on the text format it uses. That would have made this error properly visible and would properly prevent installation of a view with a missing text format. This issue is still valid, though, and the fix makes sense in my opinion.
A different solution would be to use the
text_custom
area plugin to output markup directly. I think it makes sense, though, to avoid#markup
for user-entered text, though I think the patch is good as is.Comment #7
alexpottOne thing that strikes me as a bit tricky is translations. Sometimes a translation for something can involve html to get it right. With this change I can't use html entities like
é
. See:I'm not sure what the solution here is.
Comment #8
tstoecklerWell, people who need that could simply add a proper text format, which I personally think is acceptable. The other way would be what I alluded to in #6 and simply use the custom_text area which doesn't use a format at all.
Comment #9
alexpottYep looking at \Drupal\views\Plugin\views\area\TextCustom() that is exactly what we should use.
I think applying the xss_admin filter for admin text makes sense.
Comment #10
tstoecklerAhh, I didn't realize we had the xss_admin there. I guess in that case I agree that that is the best solution as it seems to be a good tradeoff between flexibility, complexity and security.
Comment #11
dpagini CreditAttribution: dpagini as a volunteer commentedHere is my attempt at the custom_text implementation of this bug. I believe the test method is still relevant to delete here.
Comment #12
dagmarThanks!. I checked this and is working as expected. The only thing that doesn't convince me is the administrative title available in Views.
Could you change it to indicate that this is the "No recent logs message"?
Comment #13
alexpottWe should also file an issue for area plugin's to add a dependency on the text format.
Plus there is a question about the upgrade path. I think if the current active configuration matches the old configuration we should update it.
Comment #14
tstoecklerRe #13.1: See #2917817: Views text area plugin does not declare a dependency on the text format it uses which I had already opened in #6.
Comment #15
dagmar@alexpott So basically, if
Current Log message Text == 'No log messages available.'
then update the config?Comment #16
dpagini CreditAttribution: dpagini as a volunteer commentedHmm... happy to update my patch based on #12 and #13, but I think I need a little more help @alexpott. Do you know of any other issues that do something like this? Would this be an update hook in dblog? Would I manually check the view config to see if the old setting exists, and update it if it does?
Comment #17
dagmar@dpagini Thanks! There is an example of this type of upgrade path and tests here: #2851293: dblog is using the wrong views field, filter and relationships definitions
Comment #18
jhodgdonI am seeing this on my dev site.
A suggestion: I think you need to define a new text format in config/optional in the dblog module, and use that text format in the view. That way you are not depending on the text format basic_html that is defined by the standard install profile, and may not exist on all sites.
But yes, in addition, the view should know that it is depending on both the filter module and the basic_html text format. Well... it looks like you can't install Views without the filter module, so I guess that can be left out (views.info.yml lists filter module as a dependency).
Comment #19
dagmarIn my opinion, creating
filter_html
as part of the dblog module seems a lot for just a string, I think the solution proposed in #11 is the way yo go.Comment #20
jhodgdonEither way. But if for some reason the module needs a text format for its view, it should provide that format in config/optional rather than assuming a format with a particular machine name exists.
Comment #22
dagmarHere is the upgrade path with tests.
Comment #24
dagmarUsing drupal 8.4. database version this patch is smaller.
Comment #26
dagmarComment #27
dagmarIf this pass I think is ready for a final review.
Comment #28
jhodgdonQuestion: In DbLogViewsTest.php, is the parent testDBLogAndClear() method still being run if it is not overridden in the class? I am not sure exactly how the test methods to run discovery process works, so I'm not sure if we remove the override of the parent method, if it still gets recognized as a test method to run. Can we verify that?
Other than that, I think the code and test coverage look good.
Minor nitpick on the patch:
core/modules/dblog/tests/src/Functional/DbLogViewsTest.php
Docs line should start with "Tests" not "Test".
Comment #29
dagmarThanks @jhodgdon
@jhodgdon yes, by default all non private methods are inherited by child classes. Actually that method was overridden just to create the input format, now that is not required we can use the tests defined by the parent class.
Fixed.
Comment #30
jhodgdonI know that all methods (even private actually) are inherited. My question is whether the test runner, which decides which methods to run based on some criteria (like, I think they have to start with "test" and have no arguments and be public?) also runs inherited methods. Again, I don't know what the criteria for deciding which methods to run, or what the discovery process is, so I would like to see verification that this method is actually being run after having its override removed.
Comment #31
dagmar@jhodgdon sorry, my bad. Any public method starting with test will be executed.
Checking the test runner results you can check that DbLogViewsTest executed all the tests defined by DbLogTest + the one that this patch adds.
Comment #32
jhodgdonRight, duh! You can actually see the individual methods that are run in the test results, if you click the "All" link to show all (not just failing) classes.
The test results do have one coding standards error, very minor:
/var/lib/drupalci/workspace/jenkins-drupal_patches-53916/source/core/modules/dblog/tests/src/Functional/DbLogViewsTest.php
✗ 1 more line 58 The closing brace for the class must have an empty line before it
So I think if that is fixed, this will be ready to go. Thanks!
Comment #33
dagmarFixed coding standard.
Comment #34
jhodgdonOOps. I just noticed a typo: testEmtpyText() method -> empty not emtpy. :) Other than that this looks RTBC to me, so I just edited the patch to switch those two characters.
Also adding a note to the issue summary for reviewers/committers who might be confused about the test coverage.
Comment #36
catchCommitted 749b7d7 and pushed to 8.6.x. Thanks!
Comment #38
kumkum29 CreditAttribution: kumkum29 commentedis this patch included in version 8.5 ?
Thanks.
Comment #39
jhodgdonIt looks like it was only committed to 8.6, not 8.5.
Comment #40
aapokiiso CreditAttribution: aapokiiso commentedHi,
this is causing the following error with Drupal 8.6.5 and PHP 7.1 when enabling any module via drush.
The object it's complaining about is
That's the "display.default.display_options.empty.area.content" field in core/modules/dblog/config/optional/views.view.watchdog.yml. Setting the content field as an empty strings clears the error, but obviously that isn't a sufficient solution.
Comment #41
TuWebO CreditAttribution: TuWebO at Metadrop commentedHello,
Sorry about commenting again this issue, I'll leave here the comment to have it spotted. I've just run into it and I have been taking a look at the commit https://git.drupalcode.org/project/drupal/commit/749b7d7.
It looks like it is missing something, while it changes
id: area_text_custom
it is not changing the area plugin type to "area_text_custom".Something like:
I am not really familiar with configuration at this level, but installing a multi site from a custom profile with language different than english, and enabling a default_content module will make locale to run
locale_system_set_config_langcodes()
which endeds up trying to save $config->set('langcode', $default_langcode)->save() on the views.view.watchdog raising an InvalidArgumentException becausePlaceholders must have a trailing [] if they are to be expanded with an array of values.
.Changing (hardcoding for PoC) the area TO area_text_custom fixed the Exception.
Just to let you know since I've just run into it.
Comment #42
nortmas CreditAttribution: nortmas commented@TuWebO
Thank you! you're my rescuer... Couldn't find the problem during the drupal update to 8.8.1 - I had this problem in views.view.watchdog.yml Replaced to what you said and BAMM it works!
Comment #43
sibany CreditAttribution: sibany as a volunteer commented@TuWebO & @nortmas
Confirming the area TO area_text_custom fixed the issue!
thanks :)
Comment #44
apadernoComment #45
jhodgdonThis issue was fixed in 8.x actually, not sure why the version was changed? It does look like it may need a follow-up issue to fix problems...