Reduce risk of prematurely dropping meta box error messages

[barryhughes]

Changes proposed in this Pull Request:

Within the product editor—and perhaps some other admin screens we own—we use flash messaging to display errors. That is, we add error messages in one request, temporarily storing them via the options API, then retrieve and render them on a subsequent request. However, in some cases this fails:

1. Suppose a product is updated. If a validation issue is detected we will attempt to add an error which will be stored in an option as soon as we reach request shutdown.
2. Note here that when publishing/updating a product (or any post):
   • It may take several seconds for the editor to fully reload.
   • Post submissions also involve a redirect step.
3. Concurrently to this, other requests may be received (a possible origin being an admin ajax heartbeat from another tab that happens to be open) and, during that unrelated request's shutdown, the same thing will happen ... except the instance var used to temporarily store error strings will be empty, and therefore any previously stored errors will be overwritten.

How to test the changes in this Pull Request:

Owing to the nature of the problem, it's hard to contrive a test. You may or may not encounter this issue depending on how lucky or unlucky you are. However, we can at least confirm that, with this change in place, flash errors still work as expected within the product editor:

1. Go to the product editor, create a downloadable product.
2. Add a local filepath in the downloadable files area with a disallowed mimetype (the file doesn't actually need to exist—though it may need to be a suitably Windows-like filepath if you are using that OS, not entirely sure about that—do be sure to use a local path and not a remote URL, however, to trigger this particular error):

3. When the editor reloads, you should see the expected error (and in this case, that's a good thing: with this step we're just verifying that this change does not break admin error messaging):

Additionally, with reference to the tests I added, temporarily changing this line from:

$this->sut->append_to_error_store();

To:

$this->sut->save_errors();

Mimics how things previously worked (in that the save_errors() method was being called during request shutdown), and will cause that test to fail. Could be helpful as a different way to see the problem and the fix.

Other information:

This closes off one scenario, but I'd note there are other failings it doesn't touch. For instance, in a busy multi-user system one person could make an edit to a product resulting in an error, and another could open the same product for editing a split-second later and be presented with the error (could be confusing for them), which the first user would then not see (and they may then move on without addressing the problem, which they would be unaware of).

We aren't fixing that here, however. Perhaps aspects like this would be better dealt with when we rebuild more of the admin environment.

FOR PR REVIEWER ONLY:

• I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

[barryhughes]

💡 Please ignore linting errors. They either relate to lines I did not change or are false positives connected with PHP 7.0 compatibility (I created this PR to address the latter issue).

[peterfabian]

I'm pre-approving, as this PR fixes one class of problems with this code, but as noted in the description, there are more problematic scenarios that can occur, and even after this PR, the mechanism is still vulnerable to race conditions. While not perfect, I agree that this reduces the potential for errors.

I wonder if we should (can we?) hook the append_to_error_store to some other action... maybe to admin_footer or something like that? That way, the AJAX, cron and REST API requests won't interfere?

[peterfabian]

It's a bit unfortunate that this is still vulnerable to race conditions. Perhaps out of scope for this change, but I wonder if it wouldn't be better to use the self::ERROR_STORE as a prefix and store each msg with a unique string id e.g. by using uniqid and then get the relevant option only for this request... Not sure what would be the best way to pass the id around, though. 🤔

[barryhughes]

Yes ... or a keyed array, and the code responsible for generating the message would be responsible for building it in a non repetitive way (ie, the existing bad mime-type error is very long, we probably wouldn't want to repeat it 10 times for 10 problematic URLs but instead include it once and reference each URL that was an issue—or just generalize it in some other way).

[barryhughes]

Possibly user sessions and/or pinning messages to a specific user ID (see also this comment—imho we should do a deeper think about our needs and expectations).

[peterfabian]

This is still a problem, as another request can pick up the error message and deletes the option before the originating request gets to it. When I tested this, it's rather easy to trigger this by putting a breakpoint just after update option from tab/request 1, then opening another tab from the same admin and seeing the error on an unrelated screen and no error in tab 1. I wonder if we could fix this by outputting the error only if referer is the same (and storing it along with the message in the option table)? Maybe an idea for future improvement.

[barryhughes]

Agree (see also this comment).

[peterfabian]

Nice touch here!

[barryhughes]

💯

All valid points, thank you for the review (@peterfabian).

Yes, this is very much a partial fix/incremental improvement and doesn't solve all of the problems. I feel we should follow-up with more work, and perhaps it would even be a nice idea to start with an RFC as there are a few angles we might consider.

• There are of course plenty of cases where a race condition will still occur, as noted in the description as well as specific examples in your review.
• I'm keen to get at least a partial fix out in 6.5 (to reduce potential confusion as we roll out some other new functionality), but given where we are in the release cycle I'm also reluctant to increase the size of this change.
• Also, it feels like there are at least 3 different ways we might want to communicate with admin users (or users generally, even):
  • General broadcasts, where we want all users to see a message (dismissal by one should not dismiss for all).
  • Immediate broadcasts, where we want at least one user to see a message (and dismissal should be universal).
  • Targeted broadcasts, where we want the user who performed an action (ie, inside the post/product editor) and only that user to be made aware of something.
• I don't think what we have really caters to those needs (or not in a way that is obvious and fluent) and I also wonder if this is something that may even have been solved by WC Admin already, or else where that component could benefit from the same solution.

[barryhughes]

Added a task to scope further improvements to our waiting list (internal link: p7bje6-3Uh-p2).

[github-actions]

Hi @barryhughes, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

• Add the release: add testing instructions label