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-27884: Create extensibility point for views overriding in content edit and draft create #158

Merged
merged 1 commit into from Sep 22, 2017

Conversation

webhdx
Copy link
Contributor

@webhdx webhdx commented Sep 19, 2017

JIRA: https://jira.ez.no/browse/EZP-27884

Description

Changed return type of the controller's actions. It now returns View objects which are rendered by ViewManager. Templates are injected from parameters which can be easily configured. It uses the same principle as UserRegister.

@webhdx webhdx self-assigned this Sep 19, 2017
@webhdx webhdx changed the title EZP-27884 - Create extensibility point for view override in repository-forms content edit and draft create EZP-27884: Create extensibility point for views overriding in content edit and draft create Sep 20, 2017
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Found one typo and added some nitpicks. Also there's one question I have for page layout deprecation (see below).

@@ -69,7 +73,7 @@ public function __construct(
* @param int $parentLocationId Location the content should be a child of
* @param \Symfony\Component\HttpFoundation\Request $request
*
* @return \Symfony\Component\HttpFoundation\Response
* @return \EzSystems\RepositoryForms\Content\View\ContentEditView|\Symfony\Component\HttpFoundation\Response
Copy link
Member

Choose a reason for hiding this comment

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

Note: I'm not a huge fan of returning multiple unrelated object types. Of course refactoring everything now does not make sense, but please be aware of technical debt it creates when in some distant future we'll decide to use strict PHP7 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Unfortunately we can't avoid doing so. Response objects doesn't allow any extensibility because they come with rendered HTML.

* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*
* @version //autogentag//
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: we no longer use @version for anything, to be removed in new code.

- [setPagelayout, ["$pagelayout$"]]

ezrepoforms.user_register.view_templates_listener:
alias: ezrepoforms.view_templates_listener
depracated: 'The "%service_id%" service is deprecated since 1.10 and will be removed in 2.0. Use "ezrepoforms.view_templates_listener" instead.'
Copy link
Member

Choose a reason for hiding this comment

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

Typo: deprecated:

'form' => $form->createView(),
'languageCode' => $language,
'pagelayout' => $this->pagelayout,
Copy link
Member

Choose a reason for hiding this comment

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

Is this setting passed to the view somewhere else? I'm not a Twig expert, just asking :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. pagelayout is now passed via ViewTemplatesListener. This allows us to attach different pagelayout for admin UI.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, I see now :)

@webhdx
Copy link
Contributor Author

webhdx commented Sep 22, 2017

@alongosz Fixed.
@Nattfarinn Please take a look as well we need to merge this ASAP.

@lserwatka lserwatka merged commit 05afc4c into ezsystems:master Sep 22, 2017
@webhdx webhdx deleted the configurable-views branch November 13, 2017 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants