Problem/Motivation

The edit form at en/admin/config/media/menu-svg-icons/icon-set/main_nav_icons allow any kind of value for all the fields.

This have several consequences:
1. Writing an invalid SVG in the source field generate tons of errors when you access the menu edit page.
2. Its hard understand why it won't works and where is the error.
3. Its not safe don't validate at all the form even if can be used just by admins.

Proposed resolution

Implemet validation when save a new SVG source

Remaining tasks

User interface changes

An error will be reported when the source is invalid and it prevent the form to be saved.

CommentFileSizeAuthor
#7 interdiff.txt2.24 KBziomizar
#7 2922288.patch1.79 KBziomizar
#4 2922288.interdiff.txt677 bytesziomizar
#4 2922288.patch1.49 KBziomizar
#2 2922288.patch1.15 KBziomizar

Comments

ziomizar created an issue. See original summary.

ziomizar’s picture

ziomizar commented
StatusFileSize
new 2922288.patch1.15 KB

This is a first approach to solve the problem that use simplexml_load_string to see if there is some errors.
This is compatible with the function menu_svg_icons_form_menu_link_content_form_alter that use also simplexml_load_string to access values directly (trusting the value stored in the backedn without validation).

ziomizar’s picture

ziomizar commented
Status: Active » Needs review
ziomizar’s picture

ziomizar commented
StatusFileSize
new 2922288.patch1.49 KB
new 2922288.interdiff.txt677 bytes

Can be tricky also understand why the dropdown is empty in case the symbol list is empty inside the tags.
This patch include an extra check that return errors when no symbols are found in the current implementation.

Not sure if is better search in the whole SVG directly in that way

$dom->symbol

That looks more flexible. But is not part of that issue.

s_leu’s picture

s_leu commented
Status: Needs review » Needs work 
  1. +++ b/src/Form/IconSetForm.php
@@ -109,6 +109,39 @@ class IconSetForm extends EntityForm {
+    // The source field cannot be empty
+    if (empty($source)) {
+      $form_state->setErrorByName('source', $this->t('The source cannot be empty.'));
+    }

    This check seems obsolete as source is a required field in the form() method. 

  2. +++ b/src/Form/IconSetForm.php
@@ -109,6 +109,39 @@ class IconSetForm extends EntityForm {
+    // Using libxml_use_internal_errors we can iterate into the errors generated

    The comment should be "... iterate over errors generated by simplexml_load_string()".

    Or what is $source_loaded referring to exactly? 

  3. +++ b/src/Form/IconSetForm.php
@@ -109,6 +109,39 @@ class IconSetForm extends EntityForm {
+      $form_state->setErrorByName('source', $this->t('The source have to be a valid svg.'));

    The message should be "The value of source has to be a valid xml string." 

  4. +++ b/src/Form/IconSetForm.php
@@ -109,6 +109,39 @@ class IconSetForm extends EntityForm {
+    // Check if the current SVG contain at least one valid symbol definition

    Missing the full stop at the end of the comment. 

  5. +++ b/src/Form/IconSetForm.php
@@ -109,6 +109,39 @@ class IconSetForm extends EntityForm {
+            Check if the "symbol" tags are inside the "defs" tag')

    Should be "Check if there are any valid 'symbol' tags inside the 'defs' tag."
    Note the fullstop at the end of the sentence.

s_leu’s picture

s_leu commented 
+++ b/src/Form/IconSetForm.php
@@ -109,6 +109,39 @@ class IconSetForm extends EntityForm {
+      $form_state->setErrorByName('source', $this->t('The source have to be a valid svg.'));

After testing the patch I should add the following: In case libxml_get_errors() returns some errors, it would be helpful to iterate over those and print them as separate messages. This helps to fix the SVG code a lot in case it creates any problems and won't pass validation here.

ziomizar’s picture

ziomizar commented
Status: Needs work » Needs review
StatusFileSize
new 2922288.patch1.79 KB
new interdiff.txt2.24 KB

1. Removed the check.
2. This part has been refactored a bit after #6.
3. Changed as suggested.
4. and 5. full stopped.

Comment in #6 really make sense thanks.
Atm the error display line and column other than the message, but not sure how to format it to be more clear.

AndersNielsen’s picture

AndersNielsen commented
Status: Needs review » Fixed

Thank you for the effort. Pushed to dev

AndersNielsen’s picture

AndersNielsen commented
Status: Fixed » Closed (fixed)
AndersNielsen’s picture

AndersNielsen commented
Status: Closed (fixed) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.