Problem/Motivation

This is a child issue of [META] Finalise the wording of the messages in installer, toolbar and on the status report page - #2938189: [META] Finalise the wording of the messages in installer, toolbar and on the status report page

We need to agree the final text that will be visible on the status report page to let users know about this being an experimental profile.

Proposed resolution

The "before" behavior actually has a couple variations.

Regular:

If you've updated to a new version of Drupal core after installing the profile:

Proposal by David_Rothstein in the meta issue:

Remaining tasks

  1. Decide the text
  2. Implement it
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markconroy created an issue. See original summary.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
3.68 KB
ckrina’s picture

ckrina’s picture

Adding initiative tag.

ckrina’s picture

We discussed this the UX meeting:

Note that I tried to make the wording/design of the "experimental profiles" section more closely match the one for "experimental modules".

+1 to that from people attending the meeting.

I do not really see a point in the latter, especially not in making it an "error" condition and the vague reference to "issues with this site". Updating to a new version of Drupal core just means that your demo site does not match what the latest-and-greatest demo site would be (if you were to reinstall from scratch) but that is not an error condition. I can see some logic for an informational message there (rather than an error), but for now, I removed it.

+ 1 from some people in the meeting. It was said that "whatever Drupal Core does, it's not Umami's problem".

markconroy’s picture

Status: Needs review » Needs work

Suggestion at today's weekly meeting was that the text change from

Experimental profiles are provided for testing purposes only. Use at your own risk. To start building a new site, reinstall Drupal and choose a different profile.

to

Experimental profiles are provided for testing purposes only. Use at your own risk. To start building a new site, reinstall Drupal and choose a non-experimental profile.

I do not really see a point in the latter, especially not in making it an "error" condition and the vague reference to "issues with this site". Updating to a new version of Drupal core just means that your demo site does not match what the latest-and-greatest demo site would be (if you were to reinstall from scratch) but that is not an error condition. I can see some logic for an informational message there (rather than an error), but for now, I removed it.

This got a +1 from the weekly meeting today.

markconroy’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
814 bytes

Patch attached that changes to content to what we agreed at the weekly meeting. I think the rest of the work by david_rothstein here is fine.

Status: Needs review » Needs work

The last submitted patch, 7: 2938805-7.patch, failed testing. View results

Eli-T’s picture

In core/profiles/demo_umami/demo_umami.install

function demo_umami_requirements($phase) {
  $requirements = [];
  if ($phase == 'runtime') {
    $profile = drupal_get_profile();

drupal_get_profile() is deprecated - we should use \Drupal::installProfile() instead.

Also the test doesn't match the changed text.

demo_umami_requirements() outputs

'Experimental profiles are provided for testing purposes only. Use at your own risk. To start building a new site, reinstall Drupal and choose a non-experimental profile.'

'Experimental profiles are provided for testing purposes only. Use at your own risk. To start building a new site, reinstall Drupal and choose a different profile.'

Probably why it's failing.

markconroy’s picture

Status: Needs work » Needs review
FileSize
3.71 KB
1.42 KB

Adding changes from #10

Eli-T’s picture

Tested patch in #10 on SimplyTest.me

Status report shows

which is the agreed message.

A Standard install was performed with the same patch; these warnings were not shown.

Visual inspection of the deprecated code and test mismatch identified in #9 have been fixed.

Therefore this can be set to RTBC if the automated tests pass.

Eli-T’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Screenshot at #11 shows escaped <strong> tags

Eli-T’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
284.37 KB
markconroy’s picture

Yes, the name of the profile comes from the name on the installer screen which does not have the <strong> tags (when #2938185: When installing Umami, only show warning if 'Demo Umami' radio button is selected (and ensure that it is obvious that warning message only applies to the Umami profile) gets committed).

ckrina’s picture

+1 to RTBC

smaz’s picture

Just a thought, should we still store the version of Drupal that was installed so if in the future we need to bring back a warning / error about needing to re-install the demo, we can do so?

(would mean bringing back the \Drupal::state()->set('demo_umami_drupal_version', \Drupal::VERSION); part)

I don't think it would hurt to have that there for potential future use.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Eli-T’s picture

@smaz wrt #17, I don't think we can justify speculatively storing data just in case we need it in the future. I certainly can't think of anywhere else we do that, but I'm happy to be corrected.

If we have a concrete use case for requiring that data in the future, it will be to check if the version number has changed since install. At that point we can store the version number and use logic like (excuse pseudocode)

changed = false;
if (!stored_version) {
  changed = true;
}
if (stored_version && (stored_version != new_version)) {
  changed = true;
}
Eli-T’s picture

Status: Needs review » Postponed

Whilst this issue does not introduce the strong tag referenced in #13, it is due to this issue that it is exposed on the status page. Therefore I agree this should be postponed until that has been removed in #2938185: When installing Umami, only show warning if 'Demo Umami' radio button is selected (and ensure that it is obvious that warning message only applies to the Umami profile).

David_Rothstein’s picture

Status: Postponed » Reviewed & tested by the community

Whilst this issue does not introduce the strong tag referenced in #13, it is due to this issue that it is exposed on the status page.

That's not true; as can be seen from the screenshots in the issue summary, the escaped <strong> tags already appear on the status report page before this patch. This patch does increase the number of places they appear on the status report from 1 to 2, but that doesn't seem like a good reason to postpone this.

Regarding the idea of continuing to store the version of Drupal that was installed, I think the analysis in #19 is exactly correct. Also, if we did want to store this for hypothetical future purposes, I think a good argument could be made that Drupal core itself should be storing it (not the Umami profile) since other code out there could find it useful as well. So, separate issue, I think.

For those reasons, I am moving this issue back to RTBC.

  • larowlan committed 40af3f5 on 8.6.x
    Issue #2938805 by markconroy, David_Rothstein, ckrina, Eli-T: Finalise...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @DavidRothstein, I didn't realise that the escaped strong tags existed in HEAD.

Agree, this isn't making it worse.

Committed as 40af3f5 and pushed to 8.6.x

  • alexpott committed 79d9c15 on 8.5.x authored by larowlan
    Issue #2938805 by markconroy, David_Rothstein, ckrina, Eli-T: Finalise...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.