Follow-up to #2296929: Remove system_requirements() SafeMarkup::set() use
Problem/Motivation
In system_requirements(), we mark the phpversion as safe becuase it includes a link. This link is put in the 'value' field, but the purpose of the 'value' is for things like versions and amounts, see docs. The link should not be in the 'value' field, and the SafeMarkup::set should be removed.
This is related to #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set.
Proposed resolution
Put the link in the 'description' field and use a render array to handle the link. See patch in #28
OR
leave link in the value, fix that in #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set, and use a placeholders in t instead of concatenation and ::set() See patch in #37
Remaining tasks
User interface changes
Manual testing steps
*for install*
- chmod a-wx sites/default
- hack requirement condition code around the message want to test, to make it show
- install
*for /admin/reports/status *
- install
- hack requirement condition code around the message want to test, to make it show
- go to see /admin/reports/status
API changes
Comment | File | Size | Author |
---|---|---|---|
#37 | 2551725.37.patch | 884 bytes | YesCT |
#32 | 2551725.32.patch | 2.13 KB | YesCT |
#28 | interdiff.2551725.25.27.txt | 2.09 KB | YesCT |
#28 | 2551725.27.patch | 3.6 KB | YesCT |
#25 | interdiff.2551725.21.25.txt | 1.59 KB | YesCT |
Comments
Comment #2
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net commentedComment #3
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net commentedComment #4
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net commentedHere is a proposal for a resolution. It moves the link to the description, with the caveat that the description now looks cluttered. See screenshots.
BEFORE:
AFTER:
Comment #6
xjmComment #7
dorficus CreditAttribution: dorficus as a volunteer commentedThis snippet, specifically the new Url call, is causing the fail because the route to system.php is not yet initiallized. Is there a way to get to this link without needing the route initialized?
Comment #8
dorficus CreditAttribution: dorficus as a volunteer commentedFor the time being, and i'm completely open to suggestions on how to fix this, I changed the Url call to a relative url. This may not be the optimal fix, but it corrected the error and provided the correct link.
UPDATE: ignore this interdiff. it's wrong.
Comment #9
dorficus CreditAttribution: dorficus as a volunteer commentedComment #10
dorficus CreditAttribution: dorficus as a volunteer commentedNoticed an error. will fix.
Comment #11
dorficus CreditAttribution: dorficus as a volunteer commentedComment #14
YesCT CreditAttribution: YesCT commentedin head
a php key for requirements is set up.
to do something about phpinfo.
if runtime it looks like it tries to make a link to phpinfo
and if not runtime ... no link is made
and the else for phpinfo existing, happens no matter runtime or not. but, at runtime, if you dont have phpinfo, you wont see this requirements warning ...
if you also have a slightly older version of php
cause
later in the code
overwrites that same requirements php key.
I'm not sure head is behaving the way we intend it to.
maybe they should have two different requirement keys?
it seems like the logic in this could be improved. and might be out of scope for this issue...
need to look into this a bit more.
Comment #15
YesCT CreditAttribution: YesCT commentedok.
I'll open up a separate issue about the different messages using the same requirements key (and thus all that could be relevant are not all shown).
for this issue,
@dorficus and I worked through this and found that the patch in #4 took out the if runtime conditional, and we need to keep that. they are making a new patch doing that.
make each of the requirement messages show on the installer requirements screen (renaming settings file is a good trick for making install not work) and/or the admin/reports/status page (after successful install)
and for each, in each place, make a screenshot and get the markup without the patch and with the patch
in this issue we want to see no difference between head and with the patch
Comment #16
dorficus CreditAttribution: dorficus as a volunteer commentedRolled back to #4 as a base since I went about fixing this the completely wrong way. I fixed the missing "if" statement that was keeping the install from happening.
Comment #17
dorficus CreditAttribution: dorficus as a volunteer commentedComment #18
YesCT CreditAttribution: YesCT commentedopened #2552061: system_requirements() PDO warning overwrites other PHP messages
that interdiff and patch look great!
just manual testing to do.
Comment #19
stefan.r CreditAttribution: stefan.r commentedMaybe some before and after screenshots? Also the wording on the description feels a bit wonky ("For more information, view the phpinfo")
Comment #20
joelpittet@stefan.r what do you think about this?
Comment #21
YesCT CreditAttribution: YesCT commenteddoing the manual testing.
also, I think it is a bug that we cannot see the requirements page if it only contains an error and also warnings, but the requirements page is skipped in the installer if there are only warnings. so... here is an issue: #2552091: information from install requirements cannot be seen
[edit: ah, the messages are actually info, not warnings. but note the
'#markup' => t('PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.'),
is an info... might be worth git blaming that to see if it was discussed on the issue if that should be an info or a warning.
Comment #22
YesCT CreditAttribution: YesCT commentedhere is one of the messages (will do rest next)
phpinfo
head:
with patch:
---------
markup:
head:
with patch:
Comment #23
YesCT CreditAttribution: YesCT commentedphp not minimum version
head
patch
-------
markup
head
patch
-----------------
looks like we do not want this br tag.
Comment #24
YesCT CreditAttribution: YesCT commentedphp version and mysql
head
patch
markup
head
patch
---------
to make it match head, we dont need this br either.
Comment #25
YesCT CreditAttribution: YesCT commentedfixed the br's and changed the for more information wording
this needs review. I think it is ready.
Comment #26
YesCT CreditAttribution: YesCT commentedah! here is testing of the report (and the change to the more information string)
head
patch
--------
markup
head
patch
Comment #27
joelpittetThis looks really close just a couple of things I spotted while reviewing the patch:
This looks like it may append instead of override the description.
@link should be @url.
Comment #28
YesCT CreditAttribution: YesCT commenteddoh! it totally doubled up the messages. confirmed it with manual testing. and checked with this fix that the behavior with the patch is now back to head (in that the last message set is the one that gets shown).
also changed to @url.
Comment #29
joelpittetAwesome, gave this a manual test with the 4 different messages (and one in the status report page with extra info).
The screenshots still apply above except I also tested if multiple were to override each other now and they don't.
Comment #30
stefan.r CreditAttribution: stefan.r commentedYes if we don't want any markup in the value key this should be fine.
Were the last 2 chunks in that patch (about the minimum version and the multiple statement disabling) actually needed?
Also, a separate issue, but doesn't the multiple statement message overwrite the previous message, I don't think it should?
Comment #31
YesCT CreditAttribution: YesCT commented@stefan.r ah, I'll try just setting description to a string that comes from t() and not using the markup key (cause the string doesn't have markup).
about the overwriting, yes, I opened #2552061: system_requirements() PDO warning overwrites other PHP messages in #18
Comment #32
YesCT CreditAttribution: YesCT commentedso stricktly speaking, I think this is the minimum changes that were needed.
but I did really like some of the refactoring ...
Comment #33
joelpittetIt would have been nice to keep some of the duplication out of the if statement but this is minimal now.
We have a bit of a regression that I didn't spot before but the array would have helped with. Where if there is a version number was added at runtime in the description it will now be overridden and missing from the error cases.
Though in our docs we do say we should leave the 'value' key be a value only, and I'd have loved to remove the extra cruft from here we have an issue open to resolve that.
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
#2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set
So two ways forward, one continue with the 'value' key clean-up but get those render array #markup back to prepend the phpinfo link in the top of the description. Or just refactor the wording slightly and change the SafeMarkup::set() to a t() call with the version as a placeholder instead of the concatenation.
Comment #34
joelpittetOh yeah and this is NW on #33
Comment #35
willzyx CreditAttribution: willzyx commentedadditionally with changes in #33 the link to phpinfo() (if available) is not shown if PHP version allows multiple statements..
Comment #36
YesCT CreditAttribution: YesCT commentedsome screenshots of the problem.
head
patch
Comment #37
YesCT CreditAttribution: YesCT commentedthis does "Or just refactor the wording slightly and change the SafeMarkup::set() to a t() call with the version as a placeholder instead of the concatenation." from #33
verified these are all the same in head and with this patch.
install
reports
---
[edit: but need to check the reports page... when there is no phpinfo and the mysql thing...]
Comment #39
YesCT CreditAttribution: YesCT commentedwith just
same in head
and with the patch
==========
but with
...ah, it is ok. this is the same in head and with the patch
head
patch
and this weirdness seems to me to be very unlikely to actually occur for real sites, and would be fixed by #2552061: system_requirements() PDO warning overwrites other PHP messages
Comment #40
YesCT CreditAttribution: YesCT commentedComment #41
YesCT CreditAttribution: YesCT commentedfails seem like they could be unrelated. are only on the old testbot, not the new CI 2.0 one.
Here they are so we can see if they happen again:
I'll send it for a retest.
Comment #43
stefan.r CreditAttribution: stefan.r commentedThis is simpler and still gets rid of the SafeMarkup::set() call, moving the link from the value to the description can be looked at in #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set.
Tests are green and manual testing looks good, so RTBC.
Comment #44
alexpottYep this works to resolve the immediate issue and we can continue to work on whether or not this is a correct value in #2549803: hook_requirements() 'value' key is confusing and used inconsistently, and 'title' is only displayed during installation if the value is also set.
I guess one consideration is that now translators have to provide a translation for this and...
@xjm - are you happy to proceed here?
Comment #45
xjmWorks for me too. I talked to @Gábor Hojtsy and the string in #44 would be duplicated anyway already because they have different case.
So committed and pushed to 8.0.x!
Comment #47
xjmPf. ;)
Comment #49
hass CreditAttribution: hass commentedremoved, sorry wrong case.