[UX] Show CKEditor in the site status report page.

[klonos]

---

PR by @klonos: https://github.com/backdrop/backdrop/pull/2348

[klonos]

...OK, PR up for review:

• adds a new "CKEditor" entry as REQUIREMENT_INFO
• changes the jQuery entry from REQUIREMENT_OK to a REQUIREMENT_INFO (to make it bubble up with the entries for PHP/MySQL/Web server)
• removes the extraneous "version" string from various entries for simplification.

[findlabnet]

We should check, if module "CKEditor" enabled before reporting.

[klonos]

You are absolutely right @findlabnet 👍 ...I was under the impression that ckeditor.module was a system module, hence required and always on by default. This explains those Use of undefined constant CKEDITOR_VERSION exceptions in the tests too 😅

PR updated.

[jenlampton]

This doesn't look like a feature request, just a UX issue. Adjusting tags and milestones.

[klonos]

...I have split the jQuery changes off to a separate issue: #3879. Hoping to make it easier to review and approve this change here.

[jenlampton]

There still seem to be some unrelated changes in the PR: changes to how the PHP version info links are generated. Marking as NW.

[docwilmot]

Was going to push a new PR to address @jenlampton comments, but I actually agree with @klonos changes here. Brings consistency. We have:

Backdrop CMS | 1.16.x-dev              <-----NO  "version" string
PHP | Version: 5.6.31 (PHP information)             <----- "version" string
Web server | Apache/2.4.27 (Win64) PHP/5.6.31              <----- NO "version" string
jQuery | jQuery version 1.12.4 / jQuery UI version 1.12.1              <----- "version" string

Not a major issue, but I think its worth keeping and not worth a separate PR. RTBC.

[quicksketch]

I know we have a lot of similar reports in System module, so it's easy to copy/paste there. But because CKEditor is exposed by ckeditor.module, this should go in a new ckeditor_requirements() function, not in system_requirements().

[docwilmot]

This is true. So once @jenlampton agrees to leave the "Version" changes in there, will redo properly. Thanks.

[klonos]

With #3879 merged, I've updated the PR. @quicksketch you are absolutely right, but there's no need for a new function, as there is an existing ckeditor_requirements() function in ckeditor.install. With it's current implementation, that function only shows this message if no text formats have been configured to use CKEditor:

Now, the version is shown at the top of the message:

I've swapped the part that started with "Visit..." as that in combination with the fact that this is shown as a warning made the sentence sound "commanding". Starting with "To configure..." makes it sound informational/optional.

If the person viewing the status report does not have permission to edit formats, this is shown instead:

Previously, if at least one text format was configured to use CKEditor, this entire row was hidden. Now with the version shown, there's a "more" link which when clicked shows this:

...or this:

(depending on whether the person viewing the report has permission to edit formats or not)

[indigoxela]

The sandbox seems broken, it only leads to the installer. @klonos can you request a fresh one, please?

[klonos]

Done @indigoxela 👍

[stpaultim]

I tested it with a new text format. Seems to be working for me.

[indigoxela]

I like the display of CKEditor version on status page (although it ships with core anyway).

But there's one thing I don't get: why is it worth a warning if none of the text formats uses the editor? There's nothing broken or in danger then. Unlike other warnings like "Trusted host settings", which in contrast to editor usage really has security implications.

@klonos could you explain the idea behind that? Why warn if some functionality isn't used?

[klonos]

(although it ships with core anyway)

The same can be said for jQuery; we still show it in the status report 🙂

@klonos could you explain the idea behind that?

I guess to avoid WFTs when site builders new to Drupal/Backdrop enable the CKEditor module, but they don't see the toolbar show above their text areas. I'm not married to this though; it can be changed to be REQUIREMENT_INFO instead.

[indigoxela]

I'm not married to this though; it can be changed to be REQUIREMENT_INFO instead.

Personally, I'd prefer "info" then. Let's not worry admins, when nothing dangerous happens anyway.

[klonos]

Done 👍

[indigoxela]

To be honest, I don't understand the status page mapping for info and ok anyway... :wink:

The PR works as advertised and the listing of ckeditor-enabled formats is nice to have, too. Works for me.

[ghost]

(depending on whether the person viewing the report has permission to edit formats or not)

I don't like this (I left a message in the PR about this too). I don't know of anywhere else in core where we list the different roles that have access to something in order for the current user (who doesn't have access) to contact them. In fact in another issue we specifically removed all "contact your site administrator" messages...

[stpaultim]

I don't like this (I left a message in the PR about this too). I don't know of anywhere else in core where we list the different roles that have access to something in order for the current user (who doesn't have access) to contact them. In fact in another issue we specifically removed all "contact your site administrator" messages...

So, I understand and appreciate the goal to provide contextual help to point a user in the right direction to solving their problem. But, I share the concern from @BWPanda about handling this in a piecemeal fashion. Are we doing this for any other setting? Are there are status report pages items where it makes sense to do this. If so, why are we not making this decision for all of them.

My concern is that we add this here without any consensus that this is a good idea and we end up with an inconsistent and only partially implemented idea. Then the next time we are making a change to the status page, we have to have this dicussion again and/or possibily revert changes that we've already made.

If we are going to provide helpful information about which roles can update something, let's make that decision in general and not on such a specific case by case basis.

Maybe we have already had this discussion elsewhere and there is a precedent for it. If so, let's mention that.

Should this be a separate issue?

[ghost]

Should this be a separate issue?

Definitely.

[klonos]

I don't like this ... In fact in another issue we specifically removed all "contact your site administrator" messages...

Agreed. I'll remove that bit of logic from the status report.

[stpaultim]

So, my team is working on porting a Drupal 7 module to Backdrop CMS. https://www.drupal.org/project/ckeditor_blocks

The Drupal install instructions say the site must be upgraded to Ckeditor 4.3, so I went to check which version of CKEditor that we are using in core and could not find this information anywhere. Was a little surprised it was not on the status page, because it was there in Drupal 7.

I see from screenshots of this PR, that we must still be on CKEditor 4.14, so it appears I have a different problem.

[jenlampton]

The Drupal install instructions say the site must be upgraded to Ckeditor 4.3

We're on the latest, version 4.17.1: https://github.com/backdrop/backdrop/blob/1.x/core/misc/ckeditor/CHANGES.md

so it appears I have a different problem.

You might be thinking 17 is less than 3 because it starts with a 1? (That catches me sometimes too). I don't think you have a problem :)

[stpaultim]

@jenlampton - Yes, I feel very silly now. And, I just posted something in the forum asking about this same question. Now, I look silly in two places, not to mention our internal JIRA issue queue.

:-)

[jen]

Hi all! I’m @jen but not your jen.

On Jan 25, 2022, at 8:10 PM, Tim Erickson @.***> wrote:

@Jen Lampton @.***> - Yes, I feel very silly now. And, I just posted something in the forum asking about this same question. Now, I look silly in two places, not to mention our internal JIRA issue queue.

:-)

On Tue, Jan 25, 2022 at 9:54 PM Jen Lampton @.***> wrote:

The Drupal install instructions say the site must be upgraded to Ckeditor 4.3

We're on the latest, version 4.17.1: https://github.com/backdrop/backdrop/blob/1.x/core/misc/ckeditor/CHANGES.md

so it appears I have a different problem.

You might be thinking 17 is less than 3 because it starts with a 1? That catches me sometimes too. I don't think you have a problem :)

— Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/3360#issuecomment-1021837090, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAX7W66V6J6BURUFISH4CB3UX5V6LANCNFSM4GAZIKQA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

[klonos]

PR rebased and tests are green. It seems that we missed updating CKEDITOR_VERSION in #5358, so I've updated that as well. See #5476 for a follow up to automate that in the future.

[klonos]

...screenshots:

[klonos]

It seems that we missed updating CKEDITOR_VERSION ...

If #5476 is approved and gets merged before this, we should update the PR here to use ckeditor_get_version() instead of the hard-coded CKEDITOR_VERSION constant.

[ghost]

If there's no objections in that other issue and its likely to get in, we should probably do that here anyway.