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

Double-formatting in Rules import error messages #238

Open
bugfolder opened this issue Jun 23, 2024 · 2 comments · May be fixed by #239
Open

Double-formatting in Rules import error messages #238

bugfolder opened this issue Jun 23, 2024 · 2 comments · May be fixed by #239

Comments

@bugfolder
Copy link
Collaborator

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 bugfolder self-assigned this Jun 23, 2024
bugfolder added a commit to bugfolder/rules that referenced this issue Jun 23, 2024
@bugfolder
Copy link
Collaborator Author

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

Reviewed and tested. LGMT. Thanks @bugfolder!

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

Successfully merging a pull request may close this issue.

2 participants