Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
klonos opened this issue Nov 1, 2018 · 38 comments · Fixed by backdrop/backdrop#4509
Closed

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

klonos opened this issue Nov 1, 2018 · 38 comments · Fixed by backdrop/backdrop#4509

Comments

@klonos
Copy link
Member

klonos commented Nov 1, 2018

screen shot 2018-11-01 at 11 29 12 pm


PR by @klonos: backdrop/backdrop#2348

@klonos
Copy link
Member Author

klonos commented Nov 1, 2018

...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
Copy link

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

@klonos
Copy link
Member Author

klonos commented Nov 1, 2018

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 jenlampton changed the title Show CKEditor in the site status report page. [UX] Show CKEditor in the site status report page. Jan 3, 2019
@jenlampton
Copy link
Member

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

@jenlampton jenlampton modified the milestones: 1.12.0, 1.11.4 Jan 3, 2019
@jenlampton jenlampton removed this from the 1.11.4 milestone Jan 15, 2019
@jenlampton jenlampton added this to the 1.12.7 milestone Apr 27, 2019
@jenlampton jenlampton modified the milestones: 1.12.7, 1.13.1 May 15, 2019
@jenlampton jenlampton modified the milestones: 1.13.1, 1.13.2 May 23, 2019
@klonos klonos modified the milestones: 1.13.2, 1.13.3 Jun 1, 2019
@jenlampton jenlampton added this to In Progress in Rich-Text Editor Jun 21, 2019
@klonos
Copy link
Member Author

klonos commented Jun 21, 2019

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

@quicksketch quicksketch removed this from the 1.13.3 milestone Aug 7, 2019
@quicksketch quicksketch modified the milestones: 1.23.2, 1.24.1 Jan 15, 2023
@jenlampton jenlampton modified the milestones: 1.24.1, 1.24.2 Mar 15, 2023
@jenlampton jenlampton modified the milestones: 1.24.2, 1.24.3 Apr 19, 2023
@klonos klonos modified the milestones: 1.24.3, 1.25.1 Jun 6, 2023
@quicksketch quicksketch modified the milestones: 1.25.1, 1.25.2 Jun 7, 2023
@jenlampton jenlampton modified the milestones: 1.25.2, 1.26.0 Sep 5, 2023
@jenlampton
Copy link
Member

jenlampton commented Sep 5, 2023

I've reviewed the previous PR as well as one I created today, and I went with the newer one. The older one changed a lot of things not related to the task and those changes were what caused most of the change requests on the PR.

One major difference between the newer one and the older, was that the older one depended on the constant CKEDITOR_VERSION but the newer one uses the hard-coded string for the version. This means that when the version number changes, we'll need to remember to change it in two places.

I'm not sure if the constant CKEDITOR_VERSION is available when hook_requiremeents() is running, since it's defined in the .module file. .module files are not available when updates are running, and since this file contains the updates, it seems like that could be problematic.

Perhaps the runtime check will help? I would much prefer to use CKEDITOR_VERSION if possible.

@quicksketch
Copy link
Member

I left some suggestions on the PR.

@jenlampton
Copy link
Member

Suggestions merged.

@quicksketch
Copy link
Member

I merged backdrop/backdrop#4509 into 1.x and 1.25.x (just in case that gets any installs) to start including CKEditor version in the status report. Thanks everyone for their feedback, review, and code!

backdrop/backdrop@d657bfa by @jenlampton, @klonos, @kiamlaluno, @quicksketch, @docwilmot, @BWPanda, @stpaultim, and @jromine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment