EZP-29289: Migrating ezxmltext with invalid name or id attributes #47

vidarl · 2018-06-15T11:46:21Z

This PR fixes EZP-29289

It is not 100% bulletproof though;

It checks if first character in the attribute's value is white listed. It does not check the remaining characters. It would be possible to fix that, but it would require yet another XPath pass I believe.
Also, the whitelist may not be 100% complete

But I think this is good enought for now

andrerom · 2018-06-18T12:54:33Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+                'disable-id-value-check',
+                null,
+                InputOption::VALUE_NONE,
+                'Disable the check for non-validating id/name values. This might increase execution time on large databases'


decrease? As in it might speeds up execution by skipping this right?

andrerom · 2018-06-18T12:56:09Z

tests/lib/FieldType/Converter/RichTextTest.php

+
+        /*
+        // For debugging
+        $records = $logHandler->getRecords();


For convenience, I would prefer it to remain there, and this is in a test so I thought it might be okay.

But I may also remove it if needed

if we instead mock, that would cover the need for that afaik.

andrerom

+1 besides the two nitpicks above

andrerom · 2018-06-19T07:45:23Z

tests/lib/FieldType/Converter/RichTextTest.php

@@ -17,6 +17,8 @@
 use eZ\Publish\API\Repository\LocationService;
 use eZ\Publish\API\Repository\Values\Content\ContentInfo;
 use eZ\Publish\API\Repository\Values\Content\Location;
+use Monolog\Handler\TestHandler;
+use Monolog\Logger;


No test/null logger available in PSR package to avoid relying on implementation?

There is a null logger, but no test logger in PSR

But you could mock LoggerInterface here as well right? or similar.

Added mock as suggested and testes that the log messages are actually correct.

It got a bit more complicated that you might suspected as the log message in some cases contained ids generated randomly by XSLT...

andrerom · 2018-06-20T13:35:56Z

tests/lib/FieldType/Converter/RichTextTest.php

+                    $this->expectedLogWarningWithWildcard[] = $line;
+                    $loggerStub->expects($this->at($logNo++))
+                        ->method('warning')
+                    ->with($this->callback(array($this, 'validatelogWarningWithWildcard')));


I'm not entirely sure what the * log lines do here, but might be relevant:
This could have been also the following to avoid using stageful $this->expectedLogWarningWithWildcard property:

->with($this->callback(function($logMessage) use($expectedLogLines) { $this->assert(...); return true; }));

andrerom

+1, but might be improvement potential on the callback usage.

vidarl · 2018-06-20T14:20:21Z

Fixed callback usage as suggested. Thanx

andrerom · 2018-06-20T15:28:24Z

tests/lib/FieldType/Converter/RichTextTest.php

+    /**
+     * @var []
+     */
+    private $expectedLogWarningWithWildcard = [];


I guess you can remove this now then ;)

andrerom · 2018-06-20T15:28:34Z

tests/lib/FieldType/Converter/RichTextTest.php

+                    continue;
+                }
+                if (strpos($expectedLogLine, '*') !== false) {
+                    //$this->expectedLogWarningWithWildcard[] = $line;


and this ;)

andrerom · 2018-06-20T15:29:35Z

This looks great now to me.
@adamwojs or @kmadejski, could one of you have a look and review? ;)

adamwojs · 2018-06-21T07:02:33Z

lib/FieldType/XmlText/Converter/RichText.php

+            $newValue = 'rewrite_' . $node->attributes->getNamedItem('id')->nodeValue;
+            $newValue = preg_replace("/[^$whitelist]/", '_', $newValue);
+            $node->attributes->getNamedItem('id')->nodeValue = $newValue;
+            if ($this->logger !== null) {


You don't need this condition. That's why we using NullLogger 😉

Correct. Previosly, the logger could actually be null, but that is not the case anymore.

Fixed.

adamwojs · 2018-06-21T07:04:07Z

lib/FieldType/XmlText/Converter/RichText.php

@@ -163,6 +163,34 @@ protected function reportNonUniqueIds(DOMDocument $document, $contentFieldId)
        }
    }

+    protected function ValidateAttributeValues(DOMDocument $document, $contentFieldId)


CS: Please use camelCase for method names

adamwojs · 2018-06-21T07:54:15Z

lib/FieldType/XmlText/Converter/RichText.php

@@ -320,7 +348,7 @@ protected function checkEmptyEmbedTags(DOMDocument $inputDocument)
     * @param null|int $contentFieldId
     * @return string
     */
-    public function convert(DOMDocument $inputDocument, $checkDuplicateIds = false, $contentFieldId = null)
+    public function convert(DOMDocument $inputDocument, $checkDuplicateIds = false, $checkIdValues = false, $contentFieldId = null)


Nitpick: Missing checkIdValues parameter in docblock

good catch!. Fixed

adamwojs · 2018-06-21T07:57:24Z

lib/FieldType/XmlText/Converter/RichText.php

+
+        $whitelist = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_-';
+        $replaceStr = '';
+        $nodes = $xpath->query("//*[


I think we should add a comment explaining whats going on here 😉

hehe... explanation given...

vidarl · 2018-06-21T11:44:06Z

@adam: All comments accommodated I believe.. Please have a look

I'll rebase PR once you are done

kmadejski · 2018-06-21T14:16:40Z

lib/FieldType/XmlText/Converter/RichText.php

+         *  #1 not starting with a..z or '_'
+         *  #2 not a..z, '0..9', '_' or '-' after 1st character
+         * So, no xpath v2 to our disposal...
+         * 1st line : we check the 1st char(substring) in id, converts it to 'a' if it in whitelist(translate), then check if it string now starts with 'a'(starts-with), then we invert result(not)


Great to see this comment! 🙂

vidarl changed the base branch from master to move_xsl_from_kernel June 15, 2018 12:09

vidarl force-pushed the fix_ezp29289 branch from a7895b1 to 5f285d5 Compare June 15, 2018 12:12

vidarl changed the base branch from move_xsl_from_kernel to master June 15, 2018 12:12

vidarl requested review from adamwojs and andrerom June 15, 2018 12:15

vidarl force-pushed the fix_ezp29289 branch from 65f6eab to 653768b Compare June 18, 2018 10:49

andrerom reviewed Jun 18, 2018

View reviewed changes

andrerom approved these changes Jun 18, 2018

View reviewed changes

andrerom reviewed Jun 19, 2018

View reviewed changes

vidarl force-pushed the fix_ezp29289 branch from 1fcb1e6 to 926ac73 Compare June 20, 2018 13:12

andrerom reviewed Jun 20, 2018

View reviewed changes

andrerom approved these changes Jun 20, 2018

View reviewed changes

vidarl force-pushed the fix_ezp29289 branch from 2e7b082 to 343bdd9 Compare June 20, 2018 14:19

andrerom reviewed Jun 20, 2018

View reviewed changes

adamwojs requested changes Jun 21, 2018

View reviewed changes

adamwojs approved these changes Jun 21, 2018

View reviewed changes

vidarl added 2 commits June 21, 2018 16:17

EZP-29289: Migrating ezxmltext with invalid name or id attributes

2c5efc1

Testing that richtext conversion logs warnings correctly

faca7a5

kmadejski approved these changes Jun 21, 2018

View reviewed changes

vidarl force-pushed the fix_ezp29289 branch from 3650835 to faca7a5 Compare June 21, 2018 14:18

vidarl merged commit 63b05cd into master Jun 21, 2018

vidarl deleted the fix_ezp29289 branch June 21, 2018 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29289: Migrating ezxmltext with invalid name or id attributes #47

EZP-29289: Migrating ezxmltext with invalid name or id attributes #47

vidarl commented Jun 15, 2018 •

edited

Loading

andrerom Jun 18, 2018

vidarl Jun 18, 2018

andrerom Jun 18, 2018

vidarl Jun 19, 2018 •

edited

Loading

andrerom Jun 20, 2018 •

edited

Loading

andrerom left a comment •

edited

Loading

andrerom Jun 19, 2018

vidarl Jun 19, 2018

andrerom Jun 19, 2018 •

edited

Loading

vidarl Jun 20, 2018

andrerom Jun 20, 2018 •

edited

Loading

andrerom left a comment

vidarl commented Jun 20, 2018

andrerom Jun 20, 2018

vidarl Jun 21, 2018

andrerom Jun 20, 2018

vidarl Jun 21, 2018

andrerom commented Jun 20, 2018 •

edited

Loading

adamwojs Jun 21, 2018

vidarl Jun 21, 2018

adamwojs Jun 21, 2018

vidarl Jun 21, 2018

adamwojs Jun 21, 2018

vidarl Jun 21, 2018

adamwojs Jun 21, 2018

vidarl Jun 21, 2018

vidarl commented Jun 21, 2018

kmadejski Jun 21, 2018 •

edited

Loading

EZP-29289: Migrating ezxmltext with invalid name or id attributes #47

EZP-29289: Migrating ezxmltext with invalid name or id attributes #47

Conversation

vidarl commented Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidarl Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

andrerom Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

andrerom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

andrerom left a comment

Choose a reason for hiding this comment

vidarl commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom commented Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidarl commented Jun 21, 2018

kmadejski Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

vidarl commented Jun 15, 2018 •

edited

Loading

vidarl Jun 19, 2018 •

edited

Loading

andrerom Jun 20, 2018 •

edited

Loading

andrerom left a comment •

edited

Loading

andrerom Jun 19, 2018 •

edited

Loading

andrerom Jun 20, 2018 •

edited

Loading

andrerom commented Jun 20, 2018 •

edited

Loading

kmadejski Jun 21, 2018 •

edited

Loading