Update status should indicate whether installed contributed projects receive security coverage

For modules that do not have security coverage, highlight them in yellow.

I'm pretty sure yellow is already used for other things (for example, a module that has an update available, but the update isn't security-related). Also, even for a module that doesn't have security coverage, there should still be a way to indicate all the various states that are currently indicated via color: module is up-to-date, module is not up-to-date, module is not up-to-date and a security update is available, etc. So this needs some more thought.

Probably this should try to use something as similar as possible to whatever design comes out of #786702: Add information about releases covered by security advisories to project pages near download table (in order to promote consistency between what people see on drupal.org compared to what they see on their own sites)?

With the new shield icon now deployed on production d.o, I naturally expected to find this thread on adding it to update.module's admin/reports/updates (to use D7 parlance, as I'm not a D8 user yet).

Having the shield on project pages informs about whether or not to install a project; having it at admin/reports/updates would inform about whether or not to keep a project installed. For someone looking to have only covered projects installed, indicators in both of these places would be more comprehensive than just the former alone.

So that is a +1?

I am asking around to find out if this isn't soft-postponed on a theoretical issue to update the XML.

Yes, the XML needs to have the data before core can show it.

I do recommend getting a head start on implementation here if possible.

The <release> elements will get a new child with a straightforward value. Maybe <security>covered</security>/<security>not covered</security>

So that is a +1?

Yup.

<security>covered</security>

Something that simple surely makes sense, but is the word "covered" (or variations thereof) already being used in machine-readable places such as d.o's field names and views config? If not, meaning it's merely part of (translatable) strings like the shield's alt text and legend, then we might be wise to avoid it given this explanation of what the indicator really means. It's also worth considering whether or not this will be binary forever. In any case, it should be informed by any machine names that have already been put into use anywhere else.

This is as far as I got but I'm quitting for the day.

Wherever we go with styling this, it is to be determined if we simply let the value pass through to the theme layer like this, or update the update status (even adding a new status constant) as it is passed around.

Actually, a bit of text for the message would be good to include in the XML. Links would be good to be able to use; the text should be otherwise well-sanitized so hacking Drupal.org isn't (more of) a risk to individual sites.

With the D6 end of life, we ran into limitations on what we could communicate via update status, which was not a fun to figure out. See #2687957: Improve Update Status messaging for End of Life in Drupal 7(and Drupal 8 and later).

@drumm: Consider this status message or modules and themes not covered by the Security Team's disclosure policy:

Caution: Your site uses contributed modules and / or themes that are not subject to the Drupal Security Team's Responsible Disclosure policy. Production sites should exercise care in using such modules and / or themes. For more information see https://www.drupal.org/security-team.

Building upon what @miwayha wrote:

Caution: Your site is at risk. Your site uses modules and / or themes that are not subject to the Drupal Security Team's Responsible Disclosure policy. Use of such modules and / or themes may expose your site to security vulnerabilities. For more information see https://www.drupal.org/security-team.

Starting on a test fixture change

#2853696: Add security advisory coverage to update status XML added the data to updates.drupal.org. All XML generated after now will have the new elements. In addition to regenerating after commits on a dev release and making a new release, the whole thing is regenerated daily too. So everything should have the new elements tomorrow.

If the new element contains text, that’s the reason something isn’t covered and should show up in the UI. <security covered="1"/> means things are good. For example:

https://updates.drupal.org/release-history/bad_judgement/7.x
has <security>Project has not opted into security advisory coverage!</security> on every release.

https://updates.drupal.org/release-history/api/7.x
has <security>Beta releases are not covered by Drupal security advisories.</security> on beta releases.
and <security covered="1"/> on full releases.

This helps us with #2687957: Improve Update Status messaging for End of Life in Drupal 7(and Drupal 8 and later) by letting arbitrary plain text in:
https://updates.drupal.org/release-history/api/6.x
<security>Drupal 6 and earlier are not covered by Drupal security advisories!</security>

The attached patch builds on #15:

• Updated test fixture XML to match what is now in production on updates.drupal.org
• Flattened the shield even more to keep up with Drupal.org project pages.
• Added reason for not being covered from update XML.

This only shows the information for versions you might want to update to. I think the goal, wearing my security team hat, is to have some indication on the projects you have installed. We could do any of:

• Show the shield on covered projects you have installed.
• Show the reason for not being covered on projects you have installed.
• Put a warning or error at …/admin/reports/status when you have not covered projects installed.

(Simple-reroll of #18 for array code style, and being able to apply to 8.4.x today.)

@mlhess asked me to look at this from a framework manager perspective. There's not much architecture here - I like re-using the shield from d.o - that makes lots of sense. And it'll be good to contrast the security covered modules with those that are not. I definitely think it is worth adding a warning when using modules from d.o not covered by the security policy and #14 looks good in terms of copy. Obviously custom code needs to be excluded. It'd be great to add tests given the importance of putting the shileds on the right modules.

+1 to #22 given the overall trend that admins should be made aware of a lack of coverage to the same extent that they are made aware of available security updates.

I'm on the fence about that option to suppress the messages though. I suppose that option would suppress both the existing message (updates available) and the new message (not covered). I feel this could lead to an unscrupulous developer, who is at the tail end of a fixed-price contract and had one of these messages just pop up before delivery with no maintenance contract, enabling this option before the client could notice and ask what gives. Then there's an insecure site in the wild, maybe the client has some internal turnover and there's a new custodian who is then also oblivious to this option instead of figuring out that they should get maintenance, etc. etc.

Anyone using such a thing non-maliciously could just as well ignore the messages, instead.

I would suggest that a message which appears on all admin pages should be discussed in a separate issue. (That's a pretty big user interface change compared to what was originally proposed here - which is limited to the Update Status report only.)

This patch:

• Adds the warning message if you are running uncovered modules from Drupal.org. That won’t show for custom modules not available from Drupal.org.
• Adds help text explaining the security shield.
• Moves the shield, or explanation for why something is not covered, from suggested upgrades to what you are currently running. I think we we showed it in both places the page could get quite busy.

To do:
Tests
Design - I grouped security coverage with up to date status
Copyediting - is everything clearly worded and warnings of appropriate severity?
When uncovered projects are running, add the warning to admin/reports/status

This patch should fix the notices in the last test run and adds either of the following messages to …/admin/reports/status

Personally, I’m not sure we want to put a message on every page. This is important for admins to know, but I’d say it is an order of magnitude less important than running known insecure code, which is the only case we message on every page for admins, that I know of. Conceptually, it is about the same risk, but having an SA with an upgrade to do is a lot more actionable than a project with real potential to have public reports.

Fixing notices only. Not all the test fixtures have the new XML elements, it is good to test for that case.

I shortened up the last suggestion:

Currently installed modules and themes from Drupal.org receive coverage.

This patch also starts adding testing.

I found the bit of changed markup causing the failed test, and added a test for the shield appearing when a project is covered.

Fixing my failing test.

I got the tag added in ccc_update_test.1_0.xml in the wrong place in the last patch.

I don't think "Needs design review" is actually a thing that anyone looks at, but I think what we actually care about is the usability impact of the design, so tagging for those topic maintainers.

It would be best to put current screenshots in the summary, but #25 and #27 are probably accurate.

FWIW, to me, the black shield icon looks like a bad thing rather than a good one, like it might be a different kind of bad warning like the yellow triangle. But I am not a usability professional.

It would be best to put current screenshots in the summary, but #25 and #27 are probably accurate.

Done, with the issue summary template. And yes, those are current screenshots.

FWIW, to me, the black shield icon looks like a bad thing rather than a good one, like it might be a different kind of bad warning like the yellow triangle. But I am not a usability professional.

I’m happy to change it as needed. I also think some brief explanation to go with the added help text would be good.

On Drupal.org, we use CSS like filter: opacity(0.5) drop-shadow(0 0 0 #fbf03b) brightness(2); to change the color of the icon to match the background. I don’t see filter: used outside of 3rd-party things in core; I’m not sure if it is an appropriate level of cleverness for core.

About #46

FWIW, to me, the black shield icon looks like a bad thing rather than a good one, like it might be a different kind of bad warning like the yellow triangle. But I am not a usability professional.

On drupal.org, this shield is grey rather than black when it's part of an explanation. So maybe just changing it to the same grey, would make it look less like a "bad thing" and consistent with d.o.

A few questions:

1. Can we just show the shield for when something is wrong? Not for when something is right (e.g. you got security team coverage). We should not be littering icons all over this page - that will make the ones you do have to pay attention to, not stand out.
2. The explanation is a little long and verbose. What about "Not security checked".
3. We shouldn't explain the icon in the text above, if the icon + text needs explanation. There is likely something wrong with the icon and text.
4. From a design point of view, we likely need the shield to have some actual color + maybe a stripe down the middle. The current styling is not in line with other icons,
   I would typically use the same color as we use elsewhere (e.g. orange for warning = not covered, red for error = actual security fault) to denote importance.

Can we just show the shield for when something is wrong?

Drupal.org project pages have the shield as a positive thing, I wouldn’t want to reverse this within Drupal installs.

The explanation is a little long and verbose. What about "Not security checked".

The security team doesn’t really do anything that’s broadly “checking”. We encourage potential vulnerabilities to be reported to us, triage them, and work with maintainers to get a release and security announcement.

Correct, the security team does not proactively check anything (as a core function, some team members do)

We already have both positive and negative alerts on this page. I think it would be great to change the page so we only alert to what is wrong, but that is scope creep on this issue.

Drumm is correct as well, we should match the messaging on D.O.

We already have both positive and negative alerts on this page. I think it would be great to change the page so we only alert to what is wrong, but that is scope creep on this issue.

For security coverage, that actually cuts down the issue by half and solves most of Bojhan’s concerns.

We wouldn’t add the shield, so we don’t need to explain it. On the update status details, we’re only adding the specific alert if something isn’t covered.

I think it's helpful to have positive notices on the update page. It reassures site administrators that everything is in correct order on their site.

We could take a positive approach to this and use icons to inform administrators of what modules do have security coverage, while educating them in the text as to what a lack of coverage means for them.

I think we can use a shield with a checkmark to convey both "covered and currently believed to be secure" and a shield with an exclamation point to indicate "covered and has a known security vulnerability".

In that case, anything without a shield would indicate that there is no security coverage on the module. Text in bold explains what that means for the uncovered modules.

Here's an example mockup:

@mlhess @drumm I am happy to change the messaging on D.O if that needs to match whats here.

Lets take the first step and only warn, when something is wrong. I understand that it feels reassuring, to add it everywhere - but really unsure about the clutter it adds as people won't notice it as much if we have icons scattered everywhere. Honestly even the "up to date" message we have on everything is a lot, but a bit much to change now.

I am not sure about the direction this issue is taking.

Others can correct me if I'm off base here: But it is pretty important that the status page shows us three things for modules/themes:

• up-to date
• has a new version available
• has a security update available

I don't really need or want anything else. When we're building sites, most people are making a conscious decision to use a module version and don't really think/care about security coverage. Having a note at the top showing that some modules have additional security coverage is great, but I certainly don't want to see warnings on my site if one of the module versions I use isn't the recommended release. I'm more concerned about active security issues, and not about boogie man theoretical vulnerabilities because a module isn't being watched by the security team.

In fact, I'd go so far to say, if the change was made to comment #54, I'll make a module to disable that functionality. Its a super bad user experience, and gives clients the impression their site is not secure, when that is not the case. I don't think a single Drupal 8 site can use only recommended modules right now, so it'd give the impression that Drupal 8 is not secure.

So, I'll admit, I glossed over this issue, and I know we'll do whatever we're going to do regardless of my opinions however...

Trying to communicate all of this information in a single list seems confusing to me. As a long time member of the community, I understand much of this data already, but if we want someone to visit this page of their site and intuit what's going on, a single list of data is going to be difficult to comprehend. Can I suggest that we might consider leaving the existing list largely alone and adding a new list that communicates exactly which installed modules are not covered by the security team? That would, imo, communicate much more clearly and concisely about the two different sets of problems we're trying to tackle without conflating them together.

Just a suggestion. Carry on.

Eclipse

After looking at this more, I noticed that we already have a pattern for this kind of information in core with experimental modules.

Instead of adding extra information to the Updates page, why don't we list all of the modules that lack security coverage under a warning on the Status report page?

That gives the user a clear alert as to the potential problems, lists all of the modules with that issue, and follows a current ux pattern we have.

See “Addition to status report” in the issue summary, and #27. Currently, it is linking over to the update status page. We could have it list the non-covered modules out right on the update status page.

The ability to configure away these warnings has been tossed around.

This patch adds the specific modules not covered to the status report page:

The text for each module is from updates.drupal.org and can be revised at any time as we change/clarify policies, or learn how to copyedit.

I’ve left in all the work on the update status page for now. We need to decide:

• Is the status report alone sufficient?
• Or should we ditch the shields and only show negative information when necessary on the update status page?
• Do we need to keep everything?

I’ll continue work on this to add tests as we get clarification. And a settings.php configuration to ignore the warnings.

I think we can remove the explanations for the lack of coverage on the warning and instead explain on the security advisory coverage document that Dev and RC releases aren't covered. That cuts down on redundant messages for the same information. I'm not sure that users need the release information. If they choose to update to a release with security coverage they'll likely need to do much more research to understand what their options are.

That would make this look a bit closer to the Experimental Modules warning, like this:

Modules or themes without security advisory coverage found: Drupal core, Devel, Token. Learn more about Drupal.org security advisory coverage.

I think that's enough without changes to the updates page. It gives the site admin a warning when they check their status. It tells them what modules or themes are at potential risk, and gives them a link to more information about that risk and what they can do to improve the situation.

This patch adds the killswitch to settings.php:

/**
 * Hide Drupal.org security advisory policy warnings.
 *
 * By default, Update Manager module warns about modules and themes from
 * Drupal.org that are not covered by Drupal.org’s security advisory policy.
 *
 * Security issues in non-covered projects are reported to the public issue
 * queue and will not receive coordinated security announcements.
 *
 * @see https://www.drupal.org/security-advisory-policy
 */
# $settings['update_warn_drupalorg_security'] = FALSE;

I’m also updating the issue summary with an additional example installed, to show another way a release might not be covered.

Layout plugin currently hasn’t opted into the security advisory policy. (This is somewhat equivalent to running a sandbox project for security coverage.) The maintainers can make a 8.x-1.0 release without any change in coverage.

I agree the messages are repetitive, we can change them, but I do want to offer some explanation of why exactly a specific project isn’t covered without sending people to figure it out for themselves.

Normally when there is a warning in the Status Report, we also provide instructions to site administrators on how to fix the problem.

What would the instructions say in this case? All I can think of is "uninstall the modules", but that seems like a very disproportionate and unrealistic solution for most sites...

For that reason, I'm not sure that a warning message in the Status Report actually makes sense here.

I think the actual message we're trying to get across to site administrators is something like this:

"The system is not able to tell you whether these modules have known security issues or not"

That manifests itself in two ways:

1. There might be a public issue on drupal.org discussing a known security vulnerability, so it's your job as a site owner to either keep an eye out for those or otherwise just live with the risk.
2. If a newer release of the module is available, there is a chance it might contain security fixes, even if it isn't labeled as a security release in Update Status. So you have to either assume it might contain a security fix, or read/understand the release notes to find out. (Note: This is also why I think it makes sense to have this info on the Update Status page after all, since that is the page you go to in order to see what new releases are available.)

I'm not sure how to express the above via a graphical icon... but something like a shield with a question mark inside it comes to mind?

Although this is not necessarily an optional idea I think it would still be helpful to get answers to the 4 questions we ask when reviewing core ideas:

What is the problem to solve?

(Missing functionality X, prevent Y from happening, make Z easier to do)

Who is this for?

(Evaluators, site visitors, content authors, site managers, (advanced) site builders, frontend developers, backend developers, core developers, site owners, other?)

Result: what will be the outcome?

(Describe the result: how will this improve things for the stated audience(s)?)

How can we know the desired result is achieved?

(Usability test, a metric to track, feedback)

I'm especially interested in 3 and 4: what positive change are we aiming to achieve and what could we track to measure if that's what's happening?

Good questions - I added answers to them to the issue summary.

Link to the recording: https://www.youtube.com/watch?v=Q7JuMD2rrm4

I have a question here, although it may be out of scope -- our minimum version of PHP for Drupal 8 is still 5.5.9. Which is way beyond its end of life, and no longer receiving any security fixes. And yet, our status page does not warn about that at all. Why not?

IMHO, the current generation of the patch (as seen in #65) is too strong. I don't think this should be a warning. I don't even think it should include exclamation points. It should be a notice -- a thing to be aware of, if you care to know. We should be critically careful not to scare the hell out of the people who will be using non-covered modules (which might include custom modules and forks), which I'm guessing will be the overwhelming majority of even mildly complicated Drupal sites.

This is very important, especially to encouraging security coverage status in contrib modules.

The devil is in the details - just how scary and intrusive the warnings are. Too intrusive, and site maintainers will ignore the really critical stuff, because "those messages are always there." Not intrusive enough, and we won't pressure maintainers into SA coverage. In the worst case, we'll have module maintainers marking unstable code as 1.0, just to silence the warnings for their users.

Part of the problem is that we don't have any kind of intermediate error status available. Just errors (fatal), warning (action item that must be resolved), and informational. Almost every site will have to ignore this warning, if they use:

* custom sandbox modules
* a dev/alpha/beta module

Has anyone on this thread EVER built a site without any of these?

It's also a laughable comparison to the things that we DON'T warn about:

* patches, even core patches, even core patches that aren't sourced from d.o issues
* known insecure PHP versions
* known insecure library versions

There is an important difference between "this isn't subject to a responsible security disclosure process" and "this has a known security issue". One of these messages will occur on a majority of Drupal sites, and people can choose to ignore it dependent on their context. The other one of these messages is a serious warning that should block a site launch. If we don't differentiate, we are stuck either over-emphasizing one message, or under-emphasizing the other.

There should be a warning on the status page about modules without security coverage, which links to the updates page for specific information as to which modules. This is the same pattern already in use for other status messages which direct users from the message to the appropriate page for action.

First, lets identify what a 'warning' is on the status page. These are (or should be) actionable items that a site builder can take to eliminate said warning on their site. But this warning (as well as experimental modules) has no immediately actionable item, except tearing their website down to only full releases, covered by the security team. While it'd be great to think we could use these warnings to push for stable releases, the simple fact is, it won't. This issue reeks of politics (throwing a 'bone' to the security team for the project application process) and not a solid UX or technical implementation.

However, I agree that somewhere on the status page, as a notice, would be a good place to mention if the site contains modules that aren't part of security coverage. This could be a link to a page on the site listing the modules, or a page on drupal.org that goes into the nuance about security coverage.

Security coverage status should be somewhere between 'update required' and 'everythings green' in terms of loudness/scariness. We don't want to scare users with an unactionable problem, but we want them informed.

No, security coverage should not be mentioned here at all. The update status page is to be about updates and known security updates, nothing else. Security coverage doesn't fall under this, pollutes the update page, and unnecessarily will scare users.

Its also important to know that the decision to choose a module is where this messaging should be, not a constant nagging. I support (for the most part) the current messaging on drupal.org, and would suggest making a way to warn builders before enabling a module not covered by the security team.

two more issues that should be addressed before this issue moves forward

• The UX team should own this issue, not the security team. They should be a stakeholder, not the ones pushing this forward. Other users should also be involved in this decision, as it has wide implications for agencies, site builders, and customers.
• This issue should have usability testing of different scenarios on the update page and status page.

This issue is extremely important to get right. If it is done wrong, the community will make a contrib module to revert this change.

I actually think that the warnings _should_ be scary. Running alpha/beta in production is a dangerous thing. The real change that would / will fix this is that maintainers need to actually push stable releases more often and stop leaving their modules in beta for five years. And if they get more complaints from users because of this, it might actually happen.

1. There should be a warning on the status page about modules without security coverage, which links to the updates page for specific information as to which modules. This is the same pattern already in use for other status messages which direct users from the message to the appropriate page for action.

   But again, what is the "action" in this case?

   There is nothing they can do from the updates page to make the warning go away -- and nothing realistic that they can do at all to make it go away. See my comment in #66, and @japerry makes a similiar point in #78.

2. The updates status information should be refactored into columns instead of the current conglomeration of rows. There should be columns for:
   ....

   • Up-to-date status
   • Security coverage status

   ...
   background colors should either be restricted to their individual columns, or eliminated in favor of iconography.

   These are big changes that seem out of scope for this issue, and (more important) I'm worried they would put way too much focus on the security coverage status information. The security coverage indicator should not be as prominent as the up-to-date indicator, but this design change would tend to make them equally or almost equally important.

   Also, these changes seem like they would directly contradict Bojhan's concerns from #50:

   Can we just show the shield for when something is wrong? Not for when something is right (e.g. you got security team coverage). We should not be littering icons all over this page - that will make the ones you do have to pay attention to, not stand out.

   I think that putting this info on the update status page does make sense (I disagree with @japerry on this, for the reasons I mentioned in #67) but it should be a simpler change that just adds a small bit of information to the existing design.

3. There does not need to be any special messaging that follows users around. The normal 'there are issues with your site' message that directs users to the status page from the configuration admin page will suffice

   I agree with the conclusion, but don't understand the reasoning :) The "there are issues with this site" message only appears in the admin interface if there are errors with your site, not warnings (which is what you are proposing to add).

I really suggest considering a simpler design, such as the one @hestenet posted in #56 (bottom panel only). I'm copying a version of that cropped to the bottom panel here:

The text is probably too long (could just use the first sentence, or even something shorter like "No security advisory coverage"?) but in general I think this is the kind of change we're looking for in this issue. And it could use the same design regardless of whether the project has updates available or not.

Projects that do have security advisory coverage don't need an indicator at all, as others have said above -- it doesn't add any particularly useful information. Yes, it is true that this would sort of be the opposite of drupal.org (which shows positive indicators for releases that have security coverage) but I think that's fine, since the purpose is different. The drupal.org pages are primarily designed for people who are installing new projects, so it makes sense for the icons to guide them to the "correct" thing to download. The Update Status page, however, is designed for people who already have the project installed on their site, so it makes sense for the icons to highlight cases that require extra attention.

We initially discussed a table based approach with additional columns but I couldn’t make it work.

1. Changed to a “box per module” design. The table-based approach isn't really up to fitting all the different types of things that we need to show per project
2. Moved the "Includes" bit right below the module name
3. We currently have two links to release notes, one to download. I removed the explicit "relase notes" link per release. Clicking the module version link gets you there as well.
4. The security coverage info is shown as a short sentence with a link. I left out the icon so that there's no competition with the actual status info icons.
5. I didn’t look into use of color to indicate status. I'd eventually like to see color used more sparingly than the current full row background fills.

What did I miss? :)

Added a somewhat related issue.. where hopefully we'll be patching core soon to eliminate the experimental module warnings.

Looking at the screenshot #83, we see redundant , inactionable, text. As a site maintainer, what am I going to do with this? Its really unfortunate that we aren't instead showing some positive wording instead that shows which modules are covered by the security policy. Maybe even order them at the top or something.

I *knew* I messed up something :)

I'm fine with switching this to positive messaging and labelling the projects that *do* have coverage. Then we'd have to match the messaging on Drupal.org as well. (Projects without any covered releases could then still have the 1 single warning)

Happy to revisit this, but so far did not get feedback on the overall proposed design.

Attached is a patch bringing in feedback summarized by #74, #83, and #86.

The new warning on the status report page:

Project-specific details have been removed.

The updated updates status page:

• Reformatted to not be a table, this uses the admin_block theme function, which comes with the light grey background.
• Rearranged the contents to match #83 and #86. I added the “Installed version” label because I thought the first version number needed some context.

Looking better! However, I'm still going to strongly argue that having the missing security coverage piece being a warning is too much for site owners, because warnings are supposed to indicate something that is actionable/fixable. This does extend to experimental modules as well, which also has too strong of wording and should be informational, unless we can find something better (suggestions maybe?)

But lastly, and maybe most importantly: My site is telling me that those who developed the site have done something potentially wrong. Along with the experimental modules (in which layout discovery should definitely be used right now, and is experimental for bad reasons IMHO), we cannot have sites contradicting what are considered module best practices within some of largest drupal development agencies in the world.

In a few years, this might not be a big deal. But right now, in Drupal 8, we cannot have warnings like this on the status page, because they are just plain misleading.

Alternatively, similar to experimental modules, I do think having some stronger wording during install is a good idea. Its important that site builders understand the potential ramifications of installing experimental and/or pre-release modules. But once the architectural decision is made, there is little reason to keep bugging the admin/user/future user about it...

Those might be reasons for an informational message, but not a warning message. (Especially since the new design doesn't treat this as a warning condition on the Update Status page itself, just an informational condition there - why would we then go ahead and treat it as a warning elsewhere?)

I think discussion of a message on the Status Report, as well as of a confirmation message when enabling modules (from #89), would be better off discussed in a separate issue anyway.

***

As for the message on the Update Status page, it looks pretty good. However, I'm concerned that many of the changes are unrelated to this issue and not necessarily a good idea. We should not be changing things that don't need changing here, especially in Drupal 7. Two things in particular:

1. 2. Moved the "Includes" bit right below the module name

   Why? This is taking the least important information and putting it near the top. This seems especially bad for Core (which as can be seen in the screenshot from #87 has a very long "Includes" list) because it pushes the important information way down to the bottom.

2. 3. We currently have two links to release notes, one to download. I removed the explicit "relase notes" link per release. Clicking the module version link gets you there as well.

   I am concerned that no one will know that though. Personally I've always clicked on "Release Notes" myself, and I think there is some logic in having it as prominent as the "Download" link. Was the goal here to save space given the new "Covered by" text that's being added? Is there another way we can deal with that?

Comparing the overall before and after screenshots (see below) this really doesn't feel like an improvement to me. The "after" screenshot (a) has way fewer visual cues to tell you which modules are up to date and which aren't (due to the removal of the background colors) and (b) has lots of extraneous text (including the "installed version" text plus the release date on an already up-to-date module, which doesn't seem useful to me and wasn't in the design either).

Again, I think it would be better to try to make the smallest possible changes that can be made to get this issue fixed.

My previously-uploaded screenshots (in the above comment) were bad so here are better ones.

BEFORE:

AFTER:

Thanks for a more detailed review @David_Rothstein :)

I would prefer to agree on the different kinds of changes needed in this issue. Implementing and fine-tuning a warning on a confirmation screen can be a separate issue. Deciding we want to do that in the first place, that would be good to do in here.

1. I moved the "Includes" section right below the project name because it's as close to a "body" text we have for the "title" that is project name. That makes it a more logical grouping: first the information about the project (name, includes), then the actions you can take on it.
2. I never read release notes, maybe that's why :). We could also keep the label "Download" and have that link to the release notes page, there's big, green download buttons in immediate view there. It would add a click compared to the current behaviour of the download link but that may be a worthy trade off? I agree that the linked version number is relatively obscure. Then we should remove it :)

In general I reshuffled things because there are potentially quite a lot of different things to communicate per project/release. I don't think we can just append another line of text to what's there now. Doing the minimal change here would amount to simply adding yet another line of text to each release. I feel we should and can do better than that.

Remember that my proposal was a wireframe. I think we're missing a visual design treatment because without the color coding and other queues the current patch makes things less quick to scan indeed.

I did a little bit of investigation to see how the Experimental warnings UX came to be. Turns out, there was no UX or really any community review other than Alex and XJM. So I don't think we should be relying on that UX as a good idea. In fact, I've suggested we remove it: #2880374: Experimental modules and themes should not have warnings after being installed

Also - it is incorrect to say that the message is not actionable.

Disagree pretty strongly about those reasons for 'actions':

• Small developers should be getting the message when they install the modules. This is where the education should occur. Once they install the module, there is nothing they can do short of remove the feature. This is not considered an action by most people.
• First, there is no way to know what better module to use. We don't make suggestions on which modules are better than the ones they choose. Secondly, composer does warn which modules aren't covered under the security policy. But also, if a developer is building with drush/composer, we should be trusting that they've done some due diligence around their decision and not second guess it. Again, not really an action here.
• Site owners need to be able to trust their site builder to be making the correct decisions. In d8 right now, the correct decision is to use modules not covered by the security policy. For modules that later on get full release updates, they will see an actionable option under the site updates page, which is wholly appropriate. Worse, if a site owner is being told the remove a module, they might do it without knowing the consequences, which then breaks their site.

Here is an example screenshot of what we could see for typical warnings pre-launch of a site:

You'll see something familiar here, all of these, except experimental modules, are reasonably expected to be actionable I do not consider uninstalling a module a reasonable action.

I would prefer to agree on the different kinds of changes needed in this issue. Implementing and fine-tuning a warning on a confirmation screen can be a separate issue. Deciding we want to do that in the first place, that would be good to do in here.

OK, can everyone here agree on an informational message on the status report page, rather than a warning message? (That is what #2880374: Experimental modules and themes should not have warnings after being installed proposes for experimental modules also.)

We could also keep the label "Download" and have that link to the release notes page, there's big, green download buttons in immediate view there. It would add a click compared to the current behaviour of the download link but that may be a worthy trade off?

That works for me. I would not be surprised if someone complains about it, but I am OK with it myself :)

Remember that my proposal was a wireframe.

Hm, well I guess the wireframe was implemented in the patch... But if it was just supposed to be a wireframe, then yes, that makes more sense.

We could also keep the label "Download" and have that link to the release notes page, there's big, green download buttons in immediate view there. It would add a click compared to the current behaviour of the download link but that may be a worthy trade off?

That works for me.

Works for me too. Occasionally release notes pages have useful notes. Sending people to them is okay.

This patch makes that change and cleans up the tests. There are no visual changes for now.

OK, can everyone here agree on an informational message on the status report page, rather than a warning message?

Clear messaging was strongly requested from the security team point of view. For now, I left it as-is. We can wait and see where #2880374: Experimental modules and themes should not have warnings after being installed goes.

Forgot to set the status for testing.

@ifrik and I discussed another possibility, which is grouping the projects in "covered" and "not covered" buckets on the page. This way it's not necessary to mention this information for each module.

Here's a more refined *wireframe*. I put the background colors back in:

Thinking about this in terms of minimal viable changes we can make. The goal of this issue is to indicate which modules are covered by the security advisory and which ones are not. This is important information but repeating it for every listed *release* of a module would mean adding a lot of repetitive text to the page.

So:

1. Must have: Group the modules that have available updates into 2 main buckets: covered and not covered. We'd still have to design how each of those headers look. But even when we'd leave the current table based display as is, this change alone would fulfill the must-have issue goal of indicating security advisory coverage.
2. The could-have would be to move away from the table based display and have boxes per project.
3. The nice-to-have then is to rearrange the information and links within each listed release.

I'm curious to hear whether people would agree with these priorities. If so, we can then design and write the specifics for the covered/not covered headings. This diagram draws a box around each of the two buckets but not sure that's actually necessary in the visual design.

I like that change when considered in isolation.

But there are opportunity costs to breaking the page up into those buckets. If you're moving away from a plain alphabetical listing, are those really the best buckets to use? Why not group by a more important attribute instead? (For example, putting modules that aren't up-to-date, and that have security updates which need to be applied, at the top of the page would probably be more logical - and would correspond more closely to how other pages such as the Status Report page at admin/reports/status are grouped.)

If the goal is just to reduce the repetitive security advisory text, shortening the text could be a simpler way to accomplish that... something like "Has security advisory coverage", for example.

This is a work in progress, only #98 with bringing back the background colors for security update status. (Security advisory coverage does not affect the background color.)

Depends on #2887860: Allow attributes to be passed to admin blocks (admin_block theme hook). This patch replaces table formatting with admin_block theming. The table had been a single column without headers, it wasn’t really being used.

If the goal is just to reduce the repetitive security advisory text, shortening the text could be a simpler way to accomplish that... something like "Has security advisory coverage", for example.

This was done on or before #98. The copy shown, as you can see in the issue summary screenshots, is “Covered by Drupal’s security advisory policy”

The update status page with this patch has no information about the real risks of running non-covered releases.

repeating it for every listed *release* of a module would mean adding a lot of repetitive text

This information is per-release. For example, if your site has 8.x-1.0-beta3, with an upgrade to 8.x-1.0 available, that upgrade would bring your site into more security advisory coverage. Doing that upgrade might be nearly as important as an upgrade for a security announcement.

https://www.drupal.org/project/media/releases/7.x-2.0 is an example of this happening recently. That release had security improvements, but as the first stable release in the 7.x-2.x series, no security announcement was required.

Replacing the update status screenshot in the issue summary with the version from #104 with background color.

Fixing a bug where the installed version sometimes was not shown. Otherwise, equivalent to #104

Showing the installed release information had been simply showing the version string. It is now using the full data structure to match the “Recommended version/Security update/etc” options below. But, I missed a place where update_calculate_project_update_status() needed to be sure to pass on the installed version’s release information.

On grouping the page - we should make sure the update status page always highlights available updates, security and otherwise.

Lack of security advisory coverage is important too. Those updates will include security fixes that aren’t called out by a security announcement. And the public issue queues for those projects will have security issues without fixes. Even if there isn’t an actionable upgrade, there is risk you should be aware of, or paying someone to be aware of.

That said, grouping by covered/not doesn’t seem like the right approach. That seems like it is prioritizing that above updates available.

Adding use Drupal\update\UpdateManagerInterface for the class constants for coding standards.

If the goal is just to reduce the repetitive security advisory text, shortening the text could be a simpler way to accomplish that... something like "Has security advisory coverage", for example.

This was done on or before #98. The copy shown, as you can see in the issue summary screenshots, is “Covered by Drupal’s security advisory policy”

My intention was to shorten it further. The difference is only a couple words, but every word counts... I feel like this is something I learned from @yoroy at some point :)

Ok, if grouping by status is not viable then we add the information to each release. In as few words as possible :)

Agreed - at this stage it is more important that users are aware whether modules are covered by the security advice or not. So the minimum viable change is to add that text to the page as it currently is, even if it's repetitive or doesn't look pretty.

Let's provide that information to users now - and change the display once somebody comes up with a good idea.

Related issue: the Help text for the Update module then need to be changed, to mention this additional information under "Checking for available updates"

Rebasing for 8.5.x

Minor update to keep up with a change in #2887860: Allow attributes to be passed to admin blocks (admin_block theme hook)

Blocker is in, so may need a re-roll here?

No re-roll needed. My local branch rebased cleanly.

Agreed, go for it!

1. Based on the above discussion, this part definitely does not have consensus:

   +      if (Settings::get('update_warn_drupalorg_security', TRUE)) {
   +        // Check if all projects have security advisory coverage.
   +        $requirements['update_covered'] = [
   +          'title' => t('Drupal.org security advisory coverage'),
   +          'value' => t('Currently installed modules and themes from Drupal.org receive coverage.'),
   +          'description' => t('Learn more about <a href="https://www.drupal.org/security-advisory-policy">Drupal.org security advisory coverage</a>.'),
   +        ];
   +        foreach ($data as $project) {
   +          // 'security_covered' boolean makes a positive assertion of coverage.
   +          // 'security' string confirms there is no coverage. Check both so
   +          // non-www.drupal.org projects are not false positives.
   +          if (isset($project['releases'][$project['version_normalized']]) && empty($project['releases'][$project['version_normalized']]['security_covered']) && !empty($project['releases'][$project['version_normalized']]['security'])) {
   +            $requirements['update_covered']['value'] = Link::createFromRoute(t('Modules and themes without security advisory coverage found'), 'update.status');
   +            $requirements['update_covered']['severity'] = REQUIREMENT_WARNING;
   +            break;
   +          }
   +        }
   +      }
   

   A couple people have discussed above why a warning on the status page is not appropriate here. (Also, it's confusing - it points you to the update status page to look for projects that don't have security coverage, but the update status page only labels ones that do (and doesn't distinguish between ones that don't and ones that are non-drupal.org projects where it isn't relevant).

   I think it would be wise to just remove this warning and leave it for a followup issue... but if not, can we at least switch it to a REQUIREMENT_INFO (as discussed above) and make it more informative, rather than like a warning?

   Also, I do not think there should be a switch for turning this off in settings.php. That is just a crutch for not being confident that this message is actually useful for real-world sites. We should either have a message that is useful, or not have one at all.... "Decisions, not options", as they say in the WordPress world :)

2. -      case UPDATE_UNKNOWN:
   -      case UPDATE_FETCH_PENDING:
   -      case UPDATE_NOT_FETCHED:
   -      case UPDATE_NOT_SECURE:
   -      case UPDATE_REVOKED:
   -      case UPDATE_NOT_SUPPORTED:
   -        $rows[$project['project_type']][$row_key]['#attributes'] = ['class' => ['color-error']];
   +
   +      case UpdateManagerInterface::NOT_SECURE:
   +      case UpdateManagerInterface::REVOKED:
   +      case UpdateManagerInterface::NOT_SUPPORTED:
   +        $class = 'color-error';
   

   Why are some of the previous error conditions no longer being treated as errors here?

3. +          <small>Covered by Drupal’s <a href="https://www.drupal.org/security-advisory-policy">security advisory policy</a></small>
   

   What happened to the apostrophe in "Drupal's"? (Same issue other places in the patch too.)

4. Related issue: the Help text for the Update module then need to be changed, to mention this additional information under "Checking for available updates"

   I agree this is worth considering.

5. I took this screenshot with the patch applied and I think it needs some discussion:

   

   If 8.x-1.1 is a security release, then is 8.x-1.0 really "covered by Drupal’s security advisory policy" (as labeled in the screenshot)? I think the technical answer is "sort of", but I also think many people would be really confused by this. I would suggest removing the label from all but the most recent security release in this scenario.

   Second, some general issues with the design visible here are (1) the "Security update" is not well-highlighted compared to the "Installed version" (I think it's more difficult for people to quickly see what the required action here is than it is with the current design in core), and (2) I still think the "Includes" line is in too prominent of a place. Suggestion: Could "Includes" be moved back down to the bottom (where it previously was) and could "Installed version: 8.x-1.0 (2017-Apr-01)" (all on one line) be moved to where "Includes" is in the screenshot? I think that would do a better job differentiating the current installed version from the recommended updates.

Removing tags. Yoroy has reviewed this through and through.

David's concerns should be addressed, but seem quite wholesome - it would be worthwhile to jump on a call.

What happened to the apostrophe in "Drupal's"? (Same issue other places in the patch too.)

That is a manifestation of #2922638: No charset on response for patch & text files. I’ve gotten in the habit of curly quotes in text for humans, and don’t think I’ve run into problems outside of the browser.

Updated patch for David_Rothstein’s feedback is attached. I think I have everyone’s feedback taken into account now. Screenshots in the issue summary have been updated.

can we at least switch it to a REQUIREMENT_INFO

Ok, done. The message is now more informative:

Projects covered by Drupal’s <a href="https://www.drupal.org/security-advisory-policy">security advisory policy</a> will have <a href="https://www.drupal.org/security">security advisories</a> responsibly disclosed when vulnerabilities are reported to Drupal’s security team. Projects from Drupal.org that are not covered may have publicly disclosed vulnerabilities.

I do not think there should be a switch for turning this off in settings.php.

Also done.

Why are some of the previous error conditions no longer being treated as errors here?

Fixed, this was an oversight in refactoring away from deprecated global constants.

Help text for the Update module then need to be changed, to mention this additional information

Added this copy in both admin/help/update & admin/reports/updates:

Projects covered by Drupal’s <a href="https://www.drupal.org/security-advisory-policy">security advisory policy</a> will have <a href="https://www.drupal.org/security">security advisories</a> responsibly disclosed when vulnerabilities are reported to Drupal’s security team. Projects from Drupal.org that are not covered may have publicly disclosed vulnerabilities.

If 8.x-1.1 is a security release, then is 8.x-1.0 really "covered by Drupal’s security advisory policy"

I’d say yes, it is covered, it got a security announcement.

I did some rearranging based on your other feedback (see screenshot in issue summary):

• Includes moved to the bottom of each block.
• Installed version moved to the end of the list, immediately followed by Includes. Works well, since includes is specifically about the installed version.
• Bolded “Security update” to match “Recommended update”. With Installed & Includes moved down, those both are listed first.

Updating tests for a copyediting change I made: “Recommended version” → “Recommended update” to match “Security update”, which reads a bit better for me.

#128 was a spurious DrupalCI failure.

This version fixes the coding standards issue, removing an unused use statement.

We can't backport this to 8.4 because of the string changes, so removing that tag.

+++ b/core/modules/update/templates/update-version.html.twig
@@ -17,20 +17,24 @@
+    {% if attributes.class != 'project-update__version--installed' %}

logic that checks a class feels wrong.

What if someone adds an additional class name or changes the classes via preprocessing.

We should be adding a new variable via template preprocess here.

I've tagged for front-end framework manager review, as there are quite a few front end changes here.

It's wrong in a couple ways, but mainly it assumes one class: use {% if not attributes.hasClass('project-update__version--installed') %}

Passing in purposeful variable may be better though as mentioned in #131

Also, we should try to avoid name changes because people could be trying to extend the templates in their own admin themes, so changes in stable/classy like 'table' to 'content' keys can do a lot of BC damage.

This patch addresses #131 & #132. A new type key is added to the update_version theme. It can be used to determine if the version being rendered is the one already installed, or a few more types of versions that might be shown.

It is also rebased against 8.6.x.

Re #133: the contents of table and content are both render arrays to represent a set of projects. Before this patch, it uses basic table theming; the replacement is an array of admin_block, no longer literally a table.

I could see going either way on this - the replacement is still a render array, but the “table” name is starting to rot.

Test failure was due to DrupalCI system issues.

This still needs frontend framework manager review for the changes to stable templates, I'll ping @lauriii

Sorry I don't have a chance right now to give a full review but I want to post my initial thoughts so that we can try to move this issue forward.

• +++ b/core/modules/update/update.module
  @@ -245,7 +246,7 @@ function update_theme() {
  +      'variables' => ['version' => NULL, 'title' => NULL, 'attributes' => [], 'type' => NULL],
  

  We shouldn't use type as a variable name for templates since it is one of the render array keys that has special functionality attached to it. More info https://www.drupal.org/docs/8/api/render-api/render-arrays#properties

• +++ b/core/themes/stable/templates/admin/update-version.html.twig
  @@ -11,24 +11,29 @@
  + * - type: The type of the version, possible values are 'recommended', 'security', 'latest', 'dev', 'also', 'installed'.
  

  What happens if value is not provided?

• It seems like the changes made for Stable are definitely on the risky side. Are all of the changes required for Stable? If there are changes that are only required for visualizing something better, the change could be moved to Seven. Or if that is not possible, maybe we should create a new template with the changes and leave the old ones intact.

Thanks for the review, laurii.

We shouldn't use type as a variable name for templates since it is one of the render array keys that has special functionality attached to it.

Renamed to version_type

What happens if value is not provided?

All uses of this template have been updated to provide a value. I expect this is not used outside of core. The addition to update_theme() gives it a NULL default.

the change could be moved to Seven

Done. I also renamed content back to table, as recommended in #133 to make this work well.

The reverts to stable changed what testing hits, reverting parts of that to match.

I also removed an extra clearfix class that had been added to make a previous design work. It is no longer needed and keeps testing more consistent.

+++ b/core/modules/update/update.report.inc
@@ -89,7 +90,7 @@ function template_preprocess_update_report(&$variables) {
-        'content' => $projects[$type_name],
+        'table' => $projects[$type_name],

This seems like an unnecessary change. I agree that this would make sense, but this will break any potential overrides of the template. I'm happy with the patch once this gets fixed so removing the tag.

That actually is reverting a change in earlier versions of the patch. The table key no longer makes sense, but it is being kept consistent so existing templates have no necessary changes.

A minor issue: I'm seeing the text "Covered by Drupal’s security advisory policy" displayed on the update status page. It does not seem to be converted properly.

That is a symptom of #2922638: No charset on response for patch & text files. If you are using a browser to download patch files, you can manually set the text encoding under the View menu. Downloading patches with curl or wget avoids the issue too.

aha, makes sense, my bad. I did not encounter any other problems.

So there's lots of stuff in this patch that's not strictly a part of the same scope, like changing the usage of the old update status constants to the class constants, changing the string labels, correcting an incorrect test assertion message, visual changes, etc. It would be better to split these different changes into different issues so that we have the minimum changes needed for each.

+++ b/core/modules/update/templates/update-version.html.twig
@@ -11,26 +11,32 @@
+ * - version_type: The type of the version, possible values are 'recommended', 'security', 'latest', 'dev', 'also',
+ *  'installed'.

This also need to be wrapped at 80 characters. :)

BTW, @hestenet, it'd be best to only mark the issue RTBC if you're actually doing reviews of it. :) And if you are doing reviews, please document what you reviewed and how. :)

Adding a note about _update_equivalent_security_releases() to the issue summary. This issue is not the non-temporary fix to replace that function.