backdrop-contrib / rules

Porting the Rules project to Backdrop CMS.
GNU General Public License v2.0
5 stars 10 forks source link

Double-formatting in Rules import error messages #238

Open bugfolder opened 3 weeks ago

bugfolder commented 3 weeks ago

When errors happen upon importing a Rule, some errors messages include literal HTML in the error message, such as in this example (try importing the attached Rule to see this in action):

image

This happens because the form uses this call to form_set_error() to display the message, using the % placeholder to italicize the message.

      try {
        $rules_config->integrityCheck();
      } catch (RulesIntegrityException $e) {
        form_set_error('import', t('Integrity check for the imported configuration failed. Error message: %message.', array('%message' => $e->getMessage())));
      }

The message is set when the RulesIntegityException is constructed, passing the message in the constructor, and the documentation for RulesIntegrityException says that the message must be translated, i.e., passed through t(). In this case, the message comes from function RulesAbstractPlugin::integrityCheck(), which makes this call:

      throw new RulesIntegrityException(t('Unknown @plugin %name.', array('@plugin' => $this->plugin(), '%name' => $this->elementName)));

So this call, using the %name placeholder, already wraps the name in the <em...> tags, which means that the second call to t() from within form_set_error() adds a second later of <em>-wrapping, but more importantly, sanitizes the HTML that was included in the message; hence the ugly error message.

There are lots of exception messages that use % placeholders. There are only two cases of form_set_error() passing an exception message as part of the output (searching for array('%message' => $e->getMessage())), so we can fix the issue simply by changing those placeholders from %message to @message. Or perhaps, since all of those RulesIntegrityException constructors are initialized via t() and its associated sanitization, the error message has already been sanitized; we could just use a !message placeholder and skip the extra sanitization.

rules_test_demo.json

bugfolder commented 3 weeks ago

Here's the result after the PR. I also fixed a double-period that was happening, since the exception message already contains a period.

image

argiepiano commented 3 weeks ago

Reviewed and tested. LGMT. Thanks @bugfolder!