EZP-28718: Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichText #39

vidarl · 2018-04-27T08:22:47Z

This one fixes
EZP-28718 : Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichText
EZP-28767 : Create a script to fix embedded images broken after migrating from XmlText to RichText

Running bin/console ezxmltext:convert-to-richtext will ensure embedded images are tagged correctly in the resulting ezrichtext.

Running bin/console ezxmltext:convert-to-richtext --fix-embedded-images-only will only fix the embedded images on an already converted DB.

Optionally, use --image-content-types=image,thumbnail,foobar to specify which classes should considered as image classes.

Related doc pr :
ibexa/documentation-developer#268

andrerom · 2018-04-27T11:05:49Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+    protected function loginAsAdmin()
+    {
+        $userService = $this->getContainer()->get('ezpublish.api.service.user');
+        $permissionResolver = $this->getContainer()->get('date_based_publisher.permission_resolver');


date_based_publisher?

Good catch. cut&paste error.. Fixed

andrerom · 2018-04-27T11:07:26Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+                'image-content-types',
+                null,
+                InputOption::VALUE_OPTIONAL,
+                'Comma separated list of content types which are considered as images when converting embedded tags. Default value is 27'


would be better if we use identifiers, type id's have a tendency to have different values depending on install more often then identifiers.

However the command can translate it to id using dbal, so service can stay on using type id.

andrerom · 2018-04-27T14:37:41Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+                    'identifier',
+                    $literalIdentifiers
+                )
+            );


You can simplify all this, welcome to DBAL's magic PARAM_(INT|STR)_ARRAY bind type ;)

ref: https://github.com/ezsystems/ezpublish-kernel/blob/d1143080d2325cadc6ccc1e3aee55b29aecebae4/eZ/Publish/Core/FieldType/BinaryBase/BinaryBaseStorage/Gateway/DoctrineStorage.php#L269

aha.. thanx.. Cause my solution was indeed insanely cumbersome

andrerom · 2018-04-27T14:44:01Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+
+        $statement = $query->execute();
+        $result = array();
+        while ($row = $statement->fetch(PDO::FETCH_ASSOC)) {


afaik you can use return $statement->fetchAll(PDO::FETCH_COLUMN); to get what you want directly here.

andrerom · 2018-04-27T14:47:33Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+        }
+
+        $query->select('id')
+            ->from($this->dbal->quoteIdentifier('ezcontentclass'))


Regarding this, I was about to say you don't need to quote constants with DBAL, but seeing it is done in https://github.com/ezsystems/ezpublish-kernel/blob/d1143080d2325cadc6ccc1e3aee55b29aecebae4/eZ/Publish/Core/FieldType/BinaryBase/BinaryBaseStorage/Gateway/DoctrineStorage.php#L269 I'm a bit unsure.

@alongosz Did you check that when writing it? Or was it just left over from style of zeta based older db code?

You are right Andre, those constants does not need to be quoted it seems

Hmm, if identifier is the same as some SQL keyword then it still needs to be quoted, I think. But AFAIR in our system it was more of the case with column names, not tables.

On the other hand... quoting only chosen ones is bad for my Aspergers, so I tried to Quote Them All ;)

But it's up to you, for sure it would be more readable w/o quoting :)

But it's up to you, for sure it would be more readable w/o quoting :)

It's not just up to me, but I prefer readability as long as we are following:

https://www.doctrine-project.org/projects/doctrine-dbal/en/2.7/reference/portability.html#portability

https://www.doctrine-project.org/projects/doctrine-dbal/en/2.7/reference/security.html#security

https://www.doctrine-project.org/projects/doctrine-dbal/en/2.7/reference/known-vendor-issues.html#known-vendor-issues

So proposal on tables and columns: We can write like this so we never need to be in doubt, and basically never need to quote:

$query->select('c.id') ->from('ezcontentclass', 'c')

Avoids the need to do $this->dbal->quoteIdentifier('id') which is done further down here, also makes it explicit which table we act on all over. Again, optimizing for reading, cause that is what we do most of the time ;)

vidarl · 2018-04-30T09:18:36Z

Fixed all review comments I believe...

vidarl · 2018-04-30T13:44:55Z

Added more bugfixes:

added support for <embed node_id=...> (previously, only <embed object_id=...> was supported
Gracefully handling if provided node_id or object_id do not exists

andrerom · 2018-04-30T14:00:00Z

lib/FieldType/XmlText/Converter/RichText.php

+     */
+    private $currentContentFieldId;
+
+    public function __construct(LoggerInterface $logger = null, $apiRepository = null, $imageContentTypes = array())


apiRepository should be first as it's not really optional.

imageContentTypes should be either optional setter injection, or required constructor argument. Rather then both at the same time.

nitpick for new/changed code: array() => []

vidarl · 2018-05-03T08:22:38Z

lib/FieldType/XmlText/Converter/RichText.php

+                try {
+                    $location = $locationService->loadLocation($id);
+                } catch (NotFoundException $e) {
+                    $this->logger->warning("Unable to find node_id=$id, referred to in embedded tag in contentobject_attribute.id=$this->currentContentFieldId.");


andrerom · 2018-04-30T14:04:05Z

lib/FieldType/XmlText/Converter/RichText.php

+        $nodes = $xpath->query('//doc:ezembed');
+        foreach ($nodes as $node) {
+            //href is in format : ezcontent://123 or ezlocation://123
+            $href=$node->attributes->getNamedItem('href')->nodeValue;


space between ' = '

andrerom · 2018-04-30T14:06:27Z

lib/FieldType/XmlText/Converter/RichText.php

+            //href is in format : ezcontent://123 or ezlocation://123
+            $href=$node->attributes->getNamedItem('href')->nodeValue;
+            $isContentId = strpos($href, 'ezcontent') === 0;
+            $id = (int) substr($href, strrpos($href, '/')+1);


We should setup cs checking on travis here, but it won't run on this PR right now as it's against dbal branch :)

Yeah.. CS and running unit tests would be good. Who can setup that? I guess it should be adressed in a separate PR though ....

unit tests will be running as soon as you now change target branch to master and rebase for master and push. Its not running here since dbal target branch is not setup as branch to run tests for, maybe we should not restrict branch to run on somehow for pr's on pr's.

See other repos where @alongosz has added cs testing on travis for inspiration on that, like: ezsystems/ezplatform-http-cache@aa1f551

andrerom · 2018-04-30T14:10:01Z

lib/FieldType/XmlText/Converter/RichText.php

+            } else {
+                if (($classAttribute !== null) && ($node->attributes->getNamedItem('class')->nodeValue === 'ez-embed-type-image')) {
+                    $node->removeAttribute('ezxhtml:class');
+                    //$node->setAttribute('ezxhtml:class', 'ez-embed-type-image');


to be removed?

As for line above, also the class logic here forget it can be several classes like in html. Or? (OE probably did not allow it, but I1'm thinking maybe the underlying xmltext allowed it if edited raw).
At least assume it can be several when comparing richtext nodes, as it adds ez-embed-type-image to whatever was there from before.

hmm.. I added this because I thought this would be a nice feature.. but I should probably check if class is indeed ez-embed-type-image, and only remove that one (not everything) if it exists... ok then ?

the comment line should be removed, indeed

yes and yes :)

vidarl · 2018-05-03T08:25:44Z

Fixed according to all review comments, and rebased

vidarl · 2018-05-03T09:02:41Z

lib/FieldType/XmlText/Converter/RichText.php

+        $xpath = new DOMXPath($richtextDocument);
+        $ns = $richtextDocument->documentElement->namespaceURI;
+        $xpath->registerNamespace('doc', $ns);
+        $nodes = $xpath->query('//doc:ezembed');


I have to deal with ezembed-inline too....

andrerom · 2018-05-04T07:53:00Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+                    ':version'
+                )
+            )
+            ->setParameters(array(


andrerom · 2018-05-04T07:54:33Z

lib/FieldType/XmlText/Converter/RichText.php

+        if ($logger === null) {
+            $this->logger = new NullLogger();
+        } else {
+            $this->logger = $logger;


Could also be:

$this->logger = $logger instanceof LoggerInterface ? $logger : new NullLogger();

andrerom · 2018-05-04T07:56:06Z

bundle/Command/ConvertXmlTextToRichTextCommand.php

+     */
+    private $converter;
+
+    public function __construct(Connection $dbal, RichTextConverter $converter, LoggerInterface $logger = null)
    {
        parent::__construct();

        $this->dbal = $dbal;
        $this->logger = $logger;


Seems we need NullLogger here to form the looks of the new code bellow.

I'll instead remove default value from constructor, forcing a logger to be set in DI config
ok?

I think that is ok, but maybe double check with @adamwojs if there are still cases logger might not be set, as we tend to some places still allow it to be null for some reason. But that might just be baggage from earlier symfony versions, or attempt to have it as optional dependency.

andrerom · 2018-05-04T07:59:49Z

lib/FieldType/XmlText/Converter/RichText.php

    {
+        $this->currentContentFieldId = $contentFieldId;


could remove this as class property and rather inject it, to avoid global state set by method which is not a setter.

ok, passing on $contentFieldId down the call stack

andrerom · 2018-05-04T08:00:27Z

tests/lib/FieldType/Converter/RichTextTest.php

+     */
+    public function providerForTestTagEmbeddedImages()
+    {
+        $map = array();


andrerom · 2018-05-04T08:00:42Z

tests/lib/FieldType/Converter/RichTextTest.php

+
+        $inputDocument = $this->createDocument($inputFilePath);
+        $richText = new RichText($apiRepositoryStub);
+        $richText->setImageContentTypes(array(27));


it's not it seems :)

vidarl · 2018-05-30T13:05:54Z

review ping. PR should now be in compliance with all review comments

alongosz · 2018-05-30T14:50:15Z

Hmm, a month ago when I commented on building queries I didn't look at what happens here at all (slammed with work, sorry).
Are you sure this is the right approach? AFAICS update queries are not going to be seen until we clear persistence cache.
It should rather be handled by proper persistence handler. Moreover if something changes in the db we might forget that we have this command in a package. So I'm rather in favor of SPI approach. If there's no corresponding API->SPI methods, it's fine to add them to SPI\Persistence and inject here persistence handler directly.

But maybe I missed something? TL;DR
😅

andrerom · 2018-05-31T06:55:02Z

@alongosz Not sure what you are talking about here, this is migration scenario, it's something people run before booting the system normally, during upgrade. And if they don't, they need to clear all cache anyway.

So while it would be great if we had generic migrations features supported somehow cleanly via SPI (Storage Engine and FieldTypes at least), it's far far beyond the scope here and more for you and engineering team to think about for future versions (this is for existing, real world versions ;)).

vidarl · 2018-05-31T07:19:37Z

In general, there could be benefits with using persistence handler in upgrade scripts. But there are pitfalls too. Often, upgrade scripts fix an invalid DB state ( schema change for instance ). So, having API calls that are able to deal with such things would be nice to have, but not sure it is worth the effort.

my 2 cents...

alongosz

Ok, if this is intended as a migration script and user knows about clearing Symfony cache after all the operations, I have no further objections.

My point however was also for building queries outside of API or SPI. Then we need to provide BC for database schema (which we do), which leads to showstoppers when it comes to evolving application.
If we had:

database migration on upgrade (doctrine schema files WiP)
everything encapsulated in at least SPI implementation
then we wouldn't need to worry about changing database at all.

Right now even if we achieve automatic database update on upgrades, then we still have some commands that operate on raw database.

But that's a little bit out of scope here. For now, with full context, which I didn't have before - +1

Some nitpicks:

.../lib/FieldType/Converter/_fixtures/tag_embedded_images/input/002-add-ez-embed-type-image.xml

+            <ezvalue key="limit">5</ezvalue>
+        </ezconfig>
+    </ezembed>
+</section>


...b/FieldType/Converter/_fixtures/tag_embedded_images/input/003-remove-ez-embed-type-image.xml

+            <ezvalue key="limit">5</ezvalue>
+        </ezconfig>
+    </ezembed>
+</section>


tests/lib/FieldType/Converter/_fixtures/tag_embedded_images/output/001-no-change.xml

+            <ezvalue key="limit">5</ezvalue>
+        </ezconfig>
+    </ezembed>
+</section>


...lib/FieldType/Converter/_fixtures/tag_embedded_images/output/002-add-ez-embed-type-image.xml

+            <ezvalue key="limit">5</ezvalue>
+        </ezconfig>
+    </ezembed>
+</section>


.../FieldType/Converter/_fixtures/tag_embedded_images/output/003-remove-ez-embed-type-image.xml

+            <ezvalue key="limit">5</ezvalue>
+        </ezconfig>
+    </ezembed>
+</section>


andrerom

Besides small nitpic on array syntax +1

I'll merge as is

vidarl requested review from adamwojs and andrerom April 27, 2018 08:24

andrerom reviewed Apr 27, 2018

View reviewed changes

vidarl changed the title ~~EZP-28767: Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichText~~ EZP-28718: Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichText Apr 27, 2018

vidarl mentioned this pull request Apr 27, 2018

Fixed EZP-28718: Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichText ibexa/documentation-developer#268

Merged

andrerom reviewed Apr 27, 2018

View reviewed changes

vidarl force-pushed the embed_image branch 2 times, most recently from 66ab403 to 1c22fbd Compare April 30, 2018 09:18

andrerom reviewed Apr 30, 2018

View reviewed changes

vidarl force-pushed the dbal branch from 1a665ce to 916cde2 Compare April 30, 2018 14:01

andrerom reviewed Apr 30, 2018

View reviewed changes

vidarl force-pushed the embed_image branch from 2f6088a to 150c3c8 Compare May 3, 2018 08:22

vidarl changed the base branch from dbal to master May 3, 2018 08:23

vidarl force-pushed the embed_image branch from 150c3c8 to ac7d7a5 Compare May 3, 2018 08:24

vidarl mentioned this pull request May 3, 2018

Fixed EZP-29135 : Converting ezxmltext to richtext when embed tag contains neither node_id or object_id fails #41

Merged

vidarl commented May 3, 2018

View reviewed changes

vidarl force-pushed the embed_image branch from ac7d7a5 to 127e6d0 Compare May 3, 2018 09:55

andrerom suggested changes May 4, 2018

View reviewed changes

alongosz approved these changes Jun 1, 2018

View reviewed changes

vidarl added 2 commits June 4, 2018 11:42

Adding "ezxhtml:class="ez-embed-type-image" attribute to embedded images

b9d085c

Ensured consistent naming of variables

468da7b

vidarl force-pushed the embed_image branch from 506b080 to 468da7b Compare June 4, 2018 09:42

andrerom approved these changes Jun 4, 2018

View reviewed changes

andrerom merged commit ccd8455 into master Jun 4, 2018

andrerom deleted the embed_image branch June 4, 2018 10:13

vidarl mentioned this pull request Aug 20, 2018

EZP-28718: Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichTex ibexa/documentation-developer#383

Closed

EZP-28718: Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichText #39

EZP-28718: Embedded images should be handled as images instead of plain embed object after migrating from XmlText to RichText #39

Conversation

vidarl commented Apr 27, 2018 • edited

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 Apr 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Apr 27, 2018 • edited

Choose a reason for hiding this comment

andrerom Apr 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Apr 28, 2018 • edited

Choose a reason for hiding this comment

andrerom Apr 28, 2018 • edited

Choose a reason for hiding this comment

vidarl commented Apr 30, 2018

vidarl commented Apr 30, 2018 • edited

andrerom Apr 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

andrerom Apr 30, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidarl commented May 3, 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

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

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 May 30, 2018

alongosz commented May 30, 2018

andrerom commented May 31, 2018 • edited

vidarl commented May 31, 2018

alongosz left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

andrerom left a comment

Choose a reason for hiding this comment

vidarl commented Apr 27, 2018 •

edited

andrerom Apr 27, 2018 •

edited

andrerom Apr 27, 2018 •

edited

andrerom Apr 27, 2018 •

edited

andrerom Apr 28, 2018 •

edited

andrerom Apr 28, 2018 •

edited

vidarl commented Apr 30, 2018 •

edited

andrerom Apr 30, 2018 •

edited

andrerom Apr 30, 2018 •

edited

andrerom commented May 31, 2018 •

edited