Fixed actionDialog() silently removes underscores "_"

[CMTV]

Affected version

2.1+

Not a bug but unexpected and weird.

In XF\Pub\Controller\Editor class there is a actionDialog() method which loads a dialog in popup when clicking on dialog-buttons in Froala editor:

public function actionDialog()
{
   $dialog = preg_replace('/[^a-zA-Z0-9]/', '', $this->filter('dialog', 'str')); // <--- The problem is here

   $data = $this->loadDialog($dialog);
   if (!$data['template'])
   {
      // prevents errors from being logged -- must explicitly define valid dialogs
      return $this->notFound();
   }

   return $this->view($data['view'], $data['template'], $data['params']);
}

First of all, this method removes all non-alphanumerical characters:

$dialog = preg_replace('/[^a-zA-Z0-9]/', '', $this->filter('dialog', 'str'));

The problem is that it removes underscores too which are commonly used when prefixing values in addons. For example a custom dialog bar of Foo/Baz addon might have a name Foo_Baz_bar and this is okay in JavaScript. But after the line above it will turn into FooBazbar which is weird and this modified value will be used as editor_dialog hint!

---

I suggest adding an underscore in pattern: '/[^a-zA-Z0-9_]/.

OR (AND)

Update editor_dialog event description saying the event hint might not be what you expect it to be (since it removes all non-alphanumeric characters)

 

[Chris D]

We wouldn't really be able to change the format of the dialog name at this point because if we start accepting underscores and other developers have already accounted for the lack of underscores in their event hints then their listener will likely stop working.

FWIW we arguably no longer need to coerce the dialog name at all. At one point the dialog name was used as part of the template name automatically for any custom editor dialogs which is likely why we attempted to make it alphanumeric (admittedly removing underscores was slightly strict).

But as design decisions go, this is now difficult to reverse without causing other problems. So we'll just have to accept it, but the description of the code event attempts to make it more clear.

 

[XF Bug Bot]

Thank you for reporting this issue. It has now been resolved and we are aiming to include it in a future XF release (2.1.1).

Change log:

Clarify the format of the expected event hint for the editor_dialog event. We coerce the dialog name to be alphanumeric so essentially anything beyond a-z/0-9 is stripped.

Any changes made as a result of this issue being resolved may not be rolled out here until later.