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

eZ Platform v3 compatibility #12

Merged
merged 17 commits into from Jul 23, 2020
Merged

eZ Platform v3 compatibility #12

merged 17 commits into from Jul 23, 2020

Conversation

kmadejski
Copy link
Member

This PR brings compatibility for eZ Platform v3. Due to introduced changes it should be released under a different major tag as it's no longer compatible with eZ Platform v2.x and lower.

@kmadejski kmadejski requested a review from damianz5 April 21, 2020 18:56
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major version release it's a good opportunity to clean up the code base. Quick self-check list:

  • Consider to mark classes as final if it's possible (usually controllers, compiler passes etc.)
  • Apply official code style (import them from ezplatform-code-style)
  • Drop deprecated classes/methods/properties

composer.json Outdated
"guzzlehttp/guzzle": "^6.3.0",
"ezsystems/ezpublish-kernel": "^6.0.0|^7.0"
"ezsystems/ezplatform-kernel": "^1.0",
"ezsystems/ezplatform-richtext": "^2.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symfony/** dependencies are missing

@kmadejski
Copy link
Member Author

kmadejski commented May 18, 2020

@adamwojs commits from 239ba3d contain changes you have requested. I did it according to my best knowledge, but some things might still be missing.
It must be released under a new major version as there are changes (added missing return types) on the interfaces.

@lserwatka could we have a QA certification on this one, please?

@kmadejski
Copy link
Member Author

@bdunogier / @damianz5 any feedback on this one, please?

use eZ\Publish\API\Repository\Values\Content\Content;
use eZ\Publish\API\Repository\Values\ContentType\FieldDefinition;
use eZ\Publish\Core\MVC\Symfony\Locale\LocaleConverterInterface;

/**
* Class Translator.
*/
class Translator
{
use RepositoryAware;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kmadejski The trait class RepositoryAware.php is removed. The use will cause exception

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, just got it from QA as well. I'll fix it soon :)

lib/TranslatorGuard.php Outdated Show resolved Hide resolved
@micszo
Copy link
Member

micszo commented Jun 12, 2020

Hi @kmadejski ! Thanks for fixing the RepositoryAware issue. 😎

It's now possible to use this new feature on 3.0, below please find the non-blocking things I noticed.

On the UI the checkbox seems to need some more love, unless I'm missing some assets but I tried reloading translations, yarn etc.:
Screenshot 2020-06-12 at 09 00 07

In the browser console an error occurs when creating a new translation, it looks a bit different on Chrome and Firefox.

FF:
Screenshot 2020-06-12 at 09 02 25
TypeError: mapping is undefined

Chrome:
Screenshot 2020-06-12 at 09 03 04
Uncaught TypeError: Cannot read property 'google' of undefined

I'll let you know if I find anything else.

@micszo
Copy link
Member

micszo commented Jun 12, 2020

What about support for Landing Pages? 🙂 e.g. some text in a Code block. The checkbox is visible when adding a translation from Page Builder level but the magic doesn't happen.

@kmadejski
Copy link
Member Author

@micszo unfortunately, it does not support Landing Pages but it could be a nice improvement 🙂

@micszo
Copy link
Member

micszo commented Jun 12, 2020

Ok, similarly the option is present when adding translations in Form Builder. 🙂 But here it could be only applicable to placeholder values I suppose.

@kmadejski
Copy link
Member Author

@micszo I have identifier the root source of the problem you spot during recent tests. There is a problem with template overriding. This bundle tries to override one of the templates, precisely admin/content/modal/add_translation.html.twig ( https://github.com/ezsystems/ezplatform-automated-translation/blob/v3/bundle/Resources/views/themes/admin/content/modal/add_translation.html.twig) which is necessary to display a switcher (checkbox).
Since this happens within the same theme, admin, the template precedence order is taken from bundles list order defined in bundles.php. I'm unsure if this behavior is completely correct, but it is what it is. To make it work, you need to change bundles order in bundles.php and move EzSystems\EzPlatformAutomatedTranslationBundle\EzPlatformAutomatedTranslationBundle::class => ['all' => true], before EzSystems\EzPlatformAdminUiBundle\EzPlatformAdminUiBundle::class => ['all' => true],.

I have updated bundle's documentation to cover this trick in: 1b7e543

@bdunogier
Copy link
Member

I'm not surprised by the behaviour you describe, @kmadejski. I'm afraid it is "correct", as in "not a side effect", but it doesn't feel right from a user perspective. I don't know what we could do about it, but it would be great if we could find a better way than fiddling with bundles order.

@micszo
Copy link
Member

micszo commented Jul 22, 2020

@kmadejski changing bundles order resolves the problem on v3.0.6. 👍
Should it also work for v3.1.0?
With the same setup I can't get it to work on 3.1.

Update: on v3.1.1 changing order works.

@kmadejski
Copy link
Member Author

kmadejski commented Jul 23, 2020

@micszo I was testing this on v3.1.0 and it seems to be alright. What is the issue you're facing?

@micszo
Copy link
Member

micszo commented Jul 23, 2020

Yesterday on v3.1.0 the order change had no effect for me but now on v3.1.1 it works fine. Not sure what was the issue.

@kmadejski
Copy link
Member Author

@micszo Just guessing that it could be cache 😅 Thanks for testing!

@lserwatka lserwatka merged commit bd58904 into master Jul 23, 2020
@lserwatka lserwatka deleted the v3 branch July 23, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants