EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching #215

webhdx · 2018-02-16T14:46:43Z

JIRA: https://jira.ez.no/browse/EZP-28849
Requires: ezsystems/ezplatform-admin-ui#375 (to work with AdminUI)

Description

I heard from @bdunogier it was always the initial idea but no one had time to finish it. As Page Builder is meant to replace edit interface for Landing Pages we need this feature. This PR only affects draft edit action, next one will be dedicated to content create.

I'd like a solid code review as this PR might potentially introduce some BC breaks.

QA Testing

Might be hard to test by QA as this is mostly related to developer experience but I'd advise to get through content editing briefly. If the content editing screen is displayed correctly and the actions are performed as expected it should be alright.

bdunogier

That's the idea, right ! I'll leave the BC to you guys :)
I had a look, and it seems fine to me, besides one point.

The controller action should be empty. If it is not, a developer has to extend from our own controller, and this is what we wanted to avoid, and why viewAction ended up empty.

The logic in the controller action needs to be moved to the ContentEditViewBuilder.

bdunogier · 2018-02-20T11:36:43Z

lib/EventListener/ContentEditViewParametersInjectorListener.php

+    {
+        /** @var ContentEditView $view */
+        $view = $event->getView();
+        if (!$view instanceof ContentEditView) {


Should ContentEditView inherit from ContentView ? That way, you wouldn't need to reimplement this listener. You can of course have an extra one that does edit specific things.

I wouldn't mix the two. Hard to predict how someone is going to use ContentView. In some cases he wouldn't expect to get content editing view. Anyway I also removed ContentEditViewParametersInjectorListener as it's no longer needed.

webhdx · 2018-02-28T08:15:38Z

ping @Nattfarinn @mikadamczyk @ViniTou any chance for code review?

alongosz

Commits overlap with #217, AFAICS my review done there applies here as well. Hopefully I didn't miss anything new.

Emphasizing one thing mentioned also in the other PR:

alongosz · 2018-03-05T10:12:48Z

bundle/Controller/ContentEditController.php

@@ -117,7 +110,7 @@ public function createWithoutDraftAction($contentTypeIdentifier, $language, $par
     * @param string $toLanguage
     * @param \Symfony\Component\HttpFoundation\Request $request
     *
-     * @return \EzSystems\RepositoryForms\Content\View\ContentCreateDraftView|\Symfony\Component\HttpFoundation\Response
+     * @return \EzSystems\RepositoryForms\Content\View\ContentCreateDraftView|Response


There's a lot of inconsistency in PhpDoc type hinting. Please decide to use either FQCN or short name.

To my knowledge, we use fully-qualified namespaces.

While real IDEs will help navigate with short names, you don't get that option when reviewing code on github. I think it's a very valid argument.

Depends on the repository. AdminUI uses short names. Is there a way to enforce it with PHP CS Fixer? @alongosz do you know?

Is there a way to enforce it with PHP CS Fixer?

No, I already looked for it some time ago. Checked just now - nothing changed - we probably would need to contribute... :)

Ok. I've changed to FQCN.

webhdx · 2018-03-08T14:19:38Z

@mikadamczyk @ViniTou @alongosz @Nattfarinn Code review needed.

webhdx · 2018-03-08T14:22:38Z

@bdunogier Controller won't stay empty because in ViewBuilder context you don't have an access to the Request thus you can't handle the form. I can get master request from the Container but the question is - is it the proper way to do it? Looks too hardcore for my liking. I'd rather have minimal logic in the controller so that anyone can override default behavior. The View is only passing information which is needed to render the template and it's fine for me. What do you think?

bdunogier · 2018-03-08T15:59:04Z

@webhdx actually, you do get the Request with ViewBuilder Filters. It is what I had used when I had prototyped this: https://github.com/ezsystems/repository-forms/pull/62/files#diff-03ff5c01c6c75308bdf76dbeb0596a18.

alongosz

Review taken from #217 (I got lost initially due to overlapping commits and posted things there)

alongosz · 2018-03-09T08:29:15Z

lib/Content/View/Builder/ContentEditViewBuilder.php

+     * @throws \Symfony\Component\OptionsResolver\Exception\InvalidOptionsException
+     * @throws \eZ\Publish\Core\Base\Exceptions\BadStateException
+     * @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentException
+     * @throws \eZ\Publish\Core\Base\Exceptions\UnauthorizedException


@webhdx please type-hint API

alongosz · 2018-03-09T08:29:43Z

lib/Content/View/Builder/ContentEditViewBuilder.php

+     *
+     * @return \eZ\Publish\API\Repository\Values\Content\Content
+     *
+     * @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentException


@webhdx please type-hint API

alongosz · 2018-03-09T08:32:20Z

lib/Content/View/Builder/ContentEditViewBuilder.php

@@ -0,0 +1,298 @@
+<?php


Nitpick: missing PhpDoc with license and copyright

webhdx · 2018-03-09T09:22:59Z

@alongosz Fixed.
@bdunogier Ok, managed to use ViewFilter event to handle the form. Controller action is just now acting as a passthrough for the ContentEditView object.

…aft edit controller and template dispatching

webhdx added the Improvement label Feb 16, 2018

webhdx self-assigned this Feb 16, 2018

webhdx requested review from lserwatka, bdunogier, mikadamczyk, Nattfarinn, micszo and ViniTou February 16, 2018 14:46

bdunogier reviewed Feb 20, 2018

View reviewed changes

webhdx force-pushed the EZP-28849_draft_edit_dispatching branch from dd81e8f to a66d7cf Compare February 22, 2018 13:12

webhdx mentioned this pull request Mar 5, 2018

EZP-28861: Allow template and controller dispatching based on matcher configuration for Content Create #217

Merged

1 task

alongosz requested changes Mar 5, 2018

View reviewed changes

webhdx mentioned this pull request Mar 7, 2018

EZP-28314: As a v2 Editor, I want to select a SiteAccess in the Preview mode ezsystems/ezplatform-admin-ui#371

Merged

2 tasks

webhdx force-pushed the EZP-28849_draft_edit_dispatching branch 4 times, most recently from 2142110 to 185dc43 Compare March 8, 2018 14:17

webhdx mentioned this pull request Mar 8, 2018

EZP-28920: Configure content edit view in AdminUI ezsystems/ezplatform-admin-ui#375

Merged

2 tasks

alongosz requested changes Mar 9, 2018

View reviewed changes

webhdx force-pushed the EZP-28849_draft_edit_dispatching branch from 185dc43 to cc1d362 Compare March 9, 2018 09:22

EZP-28849: Allow repository forms to use View Manager for handling dr…

4025795

…aft edit controller and template dispatching

webhdx force-pushed the EZP-28849_draft_edit_dispatching branch from cc1d362 to 4025795 Compare March 9, 2018 09:26

alongosz approved these changes Mar 9, 2018

View reviewed changes

barbaragr self-assigned this Mar 9, 2018

barbaragr approved these changes Mar 9, 2018

View reviewed changes

lserwatka merged commit d059304 into ezsystems:master Mar 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching #215

EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching #215

webhdx commented Feb 16, 2018 •

edited

Loading

bdunogier left a comment

bdunogier Feb 20, 2018 •

edited

Loading

webhdx Mar 8, 2018

webhdx commented Feb 28, 2018

alongosz left a comment

alongosz Mar 5, 2018

bdunogier Mar 5, 2018

webhdx Mar 7, 2018

alongosz Mar 7, 2018

webhdx Mar 8, 2018

webhdx commented Mar 8, 2018

webhdx commented Mar 8, 2018

bdunogier commented Mar 8, 2018

alongosz left a comment

alongosz Mar 9, 2018

alongosz Mar 9, 2018

alongosz Mar 9, 2018

webhdx commented Mar 9, 2018

EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching #215

EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching #215

Conversation

webhdx commented Feb 16, 2018 • edited Loading

Description

QA Testing

bdunogier left a comment

Choose a reason for hiding this comment

bdunogier Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webhdx commented Feb 28, 2018

alongosz left a 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

webhdx commented Mar 8, 2018

webhdx commented Mar 8, 2018

bdunogier commented Mar 8, 2018

alongosz left a 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

webhdx commented Mar 9, 2018

webhdx commented Feb 16, 2018 •

edited

Loading

bdunogier Feb 20, 2018 •

edited

Loading