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

Rethink the relation of checklist checks and checksum verification #46

Closed
chesio opened this issue Jul 27, 2018 · 5 comments
Closed

Rethink the relation of checklist checks and checksum verification #46

chesio opened this issue Jul 27, 2018 · 5 comments
Assignees
Milestone

Comments

@chesio
Copy link
Owner

chesio commented Jul 27, 2018

Motivation: Now when checklist checks are executed asynchronously, it could make sense to include checksum verification as yet another check in the checklist.

Notes and questions to be considered:

  1. Does it make sense to log checksum verification alerts? In general, what check failures (and how) should be logged?
  2. Does it make sense to have separate cron jobs for checksum verification, when it would be possible to just run it within checklist monitoring?
  3. It seem that some kind of distinction between simple and complex checks is necessary as some checks (removed plugins, checksum verification) are slow to execute, because they rely on external HTTP requests, so running them asynchronously/in separate cron task is viable. On the other hand, simple checks could be executed within single AJAX request/cron task.
  4. Checksum verification (but possibly some other checks too) can produce a long error report. If it should be included in checklist, UI has to be adapted to account for this.
@chesio chesio added this to the 0.9.0 milestone Jul 27, 2018
@chesio chesio self-assigned this Jul 27, 2018
@chesio
Copy link
Owner Author

chesio commented Jul 28, 2018

Does it make sense to log checksum verification alerts? In general, what check failures (and how) should be logged?

Short answer: no.

Long answer: I believe only events that are unique and non-reproducible by their nature should be recorded in events log. Failed checksum verification check can be reproduced, while failed login attempt obviously cannot.

What should be logged though, are failures due to API connection errors. But I don't think these should be logged in the events log, rather logged as standard PHP warnings (and end up in debug.log if configured) - this is what core update mechanism does in case of connection errors too. Whether they should be also reported via notifications mechanism is an open question.

@chesio
Copy link
Owner Author

chesio commented Jul 28, 2018

Does it make sense to have separate cron jobs for checksum verification, when it would be possible to just run it within checklist monitoring?

I think it does. See also point 3. I think every check that depends on external HTTP request should be contained in its own execution thread (ie. cron task when run non-interactively or AJAX request when run interactively).

@chesio
Copy link
Owner Author

chesio commented Jul 28, 2018

It seem that some kind of distinction between simple and complex checks is necessary as some checks (removed plugins, checksum verification) are slow to execute, because they rely on external HTTP requests, so running them asynchronously/in separate cron task is viable. On the other hand, simple checks could be executed within single AJAX request/cron task.

I think this distinction should be also emphasized in UI of checklist page. One may not prefer to run checks that require talking to wordpress.org or any other external service. A rough idea is to split the checklist page in two sections: basic checks that do not require any external HTTP requests and advanced checks that do.

@chesio
Copy link
Owner Author

chesio commented Jul 28, 2018

Checksum verification (but possibly some other checks too) can produce a long error report. If it should be included in checklist, UI has to be adapted to account for this.

As a first try, maybe just display full report in jQuery UI dialog.

This task would likely need more thoughts (and iterations) to find a good solution.

@chesio
Copy link
Owner Author

chesio commented Aug 2, 2018

Ok, all concerns except the last one (possibly excessive length of checksum verification report) have been dealt with, closing.

@chesio chesio closed this as completed Aug 2, 2018
chesio added a commit that referenced this issue Aug 4, 2018
Get README up to date with changes introduced by #46 and related issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant