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

EZP-29531: Do not render layout when rendering selection and relation items #2205

Merged
merged 1 commit into from Oct 3, 2018

Conversation

6 participants
@emodric
Copy link
Contributor

emodric commented Dec 20, 2017

https://jira.ez.no/browse/EZP-29531

Without this fix, rendering relation and selection items in eZ Platform Admin UI v2 looks like this:

screenshot from 2017-12-20 18-00-31

With the fix, it looks like this:

screenshot from 2017-12-20 18-00-51

@ezrobot

This comment was marked as resolved.

Copy link
Contributor

ezrobot commented Dec 20, 2017

Tool version : PHP CS Fixer 2.7.1 Sandy Pool by Fabien Potencier and Dariusz Ruminski
Command executed php-cs-fixer --dry-run -v fix
This Pull Request does not respect PSR-2 Coding Standards, please, see the suggested diff below:

diff --git a/eZ/Bundle/EzPublishCoreBundle/DependencyInjection/Compiler/IdentityDefinerPass.php b/eZ/Bundle/EzPublishCoreBundle/DependencyInjection/Compiler/IdentityDefinerPass.php
index 68aeadd..e05aabd 100644
--- a/eZ/Bundle/EzPublishCoreBundle/DependencyInjection/Compiler/IdentityDefinerPass.php
+++ b/eZ/Bundle/EzPublishCoreBundle/DependencyInjection/Compiler/IdentityDefinerPass.php
@@ -13,9 +13,7 @@ use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
 
 /**
- * Class IdentityDefinerPass
- *
-Use FOSHttpCacheBundle user context feature instead. Will be removed in future 7.x FT release.
+ * Class IdentityDefinerPass.
  */
 class IdentityDefinerPass implements CompilerPassInterface
 {

@andrerom andrerom requested review from bdunogier and Nattfarinn Dec 20, 2017

@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Dec 21, 2017

Ping :)

It would help a lot if this could land into 7.0 final :)

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Dec 21, 2017

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Dec 21, 2017

I'm fine with this. I don't see any reason why layout should be used here.

@webhdx

This comment has been minimized.

Copy link
Contributor

webhdx commented Dec 21, 2017

Hello @emodric,
We have pending PR in AdminUI which fixes this: ezsystems/ezplatform-admin-ui#262

@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Dec 21, 2017

@webhdx Great! But since these are generic templates used basically everywhere, it should be fixed here too.

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Dec 21, 2017

@emodric But you pointed out AdminUI, not a frontend template. What other uses do you have? AdminUI was resolved in the given PR.

@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Dec 21, 2017

I pointed out admin ui specifically, because we have frontensld templates overriden with this fix. But the issue stands.

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Dec 21, 2017

Right not, those templates are a bit outdated to be honest. Fixing one, does not solve all problems we have here. I would rather fix all of them as part of bigger story for 7.1 kernel.

@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Dec 21, 2017

As you wish. My point is that we as most of our client override them to fix this issue. Hence the PR

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Dec 21, 2017

The thing is, that we are very late in the process now. We are tagging final tomorrow. We can prioritise story for basic field types core templates for next release and include fix like this.

@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Dec 21, 2017

No problem, I understand :)

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Dec 21, 2017

Closing for now. We will get back to this change as part of bigger for story for core base templates.

@lserwatka lserwatka closed this Dec 21, 2017

@andrerom andrerom reopened this Aug 13, 2018

@andrerom andrerom changed the base branch from 7.0 to 7.2 Aug 13, 2018

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Aug 13, 2018

@lserwatka re-opening this, as this template is also used on front for field type rendering (the changes in content_fields.html.twig) making sure they correctly use the layout parameter is good both in terms of use as as well as making sure people don't think noLayout is what is supposed to be used on this side of things. Hence making sure the layout => noLayout parameter conversion works as intended.

@emodric can you rebase on 7.2?

@emodric emodric force-pushed the emodric:no_layout_render branch from bfe362c to 9cee529 Aug 13, 2018

@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Aug 13, 2018

@andrerom Done.

@andrerom

This comment was marked as off-topic.

Copy link
Member

andrerom commented Aug 13, 2018

@emodric Also seems change in fielddefinition_settings.html.twig is already covered in #1885, so maybe we can drop it here.

@emodric

This comment was marked as outdated.

Copy link
Contributor Author

emodric commented Aug 13, 2018

Or you can close #1885 to keep all fixes in one place :)

@andrerom

This comment was marked as outdated.

Copy link
Member

andrerom commented Aug 13, 2018

Or you can close #1885 to keep all fixes in one place :)

1885 affects (directly) 1.x, and has a jira issue specifically for it, so lower effort to separate them.

@emodric emodric changed the title Do not render layout when rendering selection and relation items EZP-29531: Do not render layout when rendering selection and relation items Aug 13, 2018

@emodric

This comment was marked as outdated.

Copy link
Contributor Author

emodric commented Aug 13, 2018

As I can see now, this is a complete duplicate of #1885, because the same issue exists in 6.7/6.13 for both of the files changed here, only #1885 is not complete. We can close either of these. If we're going to use #1885, it needs an update to include other fixes. If we're going to use this one, it can maybe be rebased to 6.7/6.13?

@andrerom

This comment was marked as outdated.

Copy link
Member

andrerom commented Aug 13, 2018

Ok

@emodric

This comment was marked as resolved.

Copy link
Contributor Author

emodric commented Aug 13, 2018

@andrerom So, what do you want me to do? :)

@emodric emodric force-pushed the emodric:no_layout_render branch from 9cee529 to 6e8dbd0 Aug 13, 2018

@alongosz
Copy link
Member

alongosz left a comment

Yeah, let's fix it. But why not for 6.7? ping @andrerom

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Aug 13, 2018

This one should be for 6.7 now as the other PR was closed, you fix @emodric ?

@emodric emodric force-pushed the emodric:no_layout_render branch from 6e8dbd0 to 550d25b Aug 13, 2018

@emodric emodric changed the base branch from 6.13 to 6.7 Aug 13, 2018

@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Aug 13, 2018

6.7 is fine ofcourse :) Rebased.

emodric added a commit to netgen/site-bundle that referenced this pull request Aug 18, 2018

Add override for ezobjectrelation_field and ezobjectrelationlist_field
blocks to disable rendering the entire layout

Part of the declined PR at ezsystems/ezpublish-kernel#2205
@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Aug 20, 2018

So, is this going in? :)

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Oct 3, 2018

🤕 Forgot about this one, sorry for the delay, but yes this is going in 👍

@andrerom andrerom merged commit 12d0525 into ezsystems:6.7 Oct 3, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
ezrobot/phpcsfixer Code review by ezrobot
Details
@emodric

This comment has been minimized.

Copy link
Contributor Author

emodric commented Oct 3, 2018

@andrerom Better late than never :) Is it going to end up in 7.3 final?

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Oct 3, 2018

Yes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.