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-28861: Allow template and controller dispatching based on matcher configuration for Content Create #217

Merged
merged 1 commit into from
Mar 12, 2018

Conversation

webhdx
Copy link
Contributor

@webhdx webhdx commented Feb 22, 2018

@webhdx webhdx self-assigned this Feb 22, 2018
@webhdx webhdx changed the title [WIP] EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching Feb 28, 2018
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.

Mostly nitpicking 🙃

*
* @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentType
* @throws \eZ\Publish\API\Repository\Exceptions\UnauthorizedException
* @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
Copy link
Member

Choose a reason for hiding this comment

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

This method can still throw NotFoundException, so you shouldn't skip it.

* @param \Symfony\Component\HttpFoundation\Request $request
* @param ContentCreateView $view
Copy link
Member

Choose a reason for hiding this comment

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

Please keep consistency when hinting types, either FQCN or not. :) I'd suggest FQCN due to existing code, but it's up to you.

$contentId,
$versionNo = null,
$language = null,
$locationId = null
Copy link
Member

Choose a reason for hiding this comment

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

Missing PhpDoc for this argument.

use Symfony\Component\DependencyInjection\ContainerBuilder;

/**
* Compiler pass to register Limitation value mappers.
Copy link
Member

Choose a reason for hiding this comment

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

That seems like copy-paste error ;)

Copy link
Member

Choose a reason for hiding this comment

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

@webhdx this hasn't been addressed.

@@ -22,7 +22,7 @@ public function addSemanticConfig(NodeBuilder $nodeBuilder)
{
$nodeBuilder
->arrayNode('content_edit')
->info('Content edit configuration')
->info('Content edit configuration. This key was deprecated in 2.1 and will be removed in 3.0. Please use siteaccess aware content_create_view key.')
Copy link
Member

Choose a reason for hiding this comment

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

Please use ->deprecated() call for that (see other usages for deprecation message format).

*/
private function loadLocation($locationId)
{
return $this->repository->getLocationService()->loadLocation($locationId);
Copy link
Member

Choose a reason for hiding this comment

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

Based on other ViewBuilder classes, this makes me think it belongs to common base or trait.

}

/**
* @return $contentType
Copy link
Member

Choose a reason for hiding this comment

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

return type-hint typo

}

/**
* @return Location
Copy link
Member

Choose a reason for hiding this comment

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

Location|null or is ?Location also correct for PhpDoc? (don't know).

use Symfony\Component\HttpKernel\Controller\ControllerReference;

/**
* Base for View Providers.
Copy link
Member

Choose a reason for hiding this comment

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

Copy-paste typo.

use Symfony\Component\HttpKernel\Controller\ControllerReference;

/**
* Base for View Providers.
Copy link
Member

Choose a reason for hiding this comment

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

Copy-paste typo.

@webhdx webhdx changed the title EZP-28849: Allow repository forms to use View Manager for handling draft edit controller and template dispatching EZP-28861: Allow template and controller dispatching based on matcher configuration for Content Create Mar 5, 2018
@webhdx webhdx force-pushed the EZP-28861_content_view_config branch 2 times, most recently from 816f060 to 6de1316 Compare March 8, 2018 13:59
@ezsystems ezsystems deleted a comment from ezrobot Mar 8, 2018
@webhdx webhdx force-pushed the EZP-28861_content_view_config branch 2 times, most recently from ca74e45 to 08de72e Compare March 8, 2018 14:16
@webhdx
Copy link
Contributor Author

webhdx commented Mar 8, 2018

@mikadamczyk @Nattfarinn @ViniTou @alongosz Code review needed. Please review only 08de72e commit as it's based on #215 for the time being.

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.

This might need a rebase, added also review to #215
One thing left here:

*
* @throws \eZ\Publish\API\Repository\Exceptions\UnauthorizedException
* @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException
* @throws \eZ\Publish\Core\Base\Exceptions\InvalidArgumentException
Copy link
Member

Choose a reason for hiding this comment

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

@webhdx please type-hint API.

@webhdx webhdx force-pushed the EZP-28861_content_view_config branch 2 times, most recently from 2786762 to c19d7d7 Compare March 12, 2018 12:53
@webhdx webhdx force-pushed the EZP-28861_content_view_config branch from c19d7d7 to 66cd116 Compare March 12, 2018 12:54
@lserwatka lserwatka merged commit 801d9ef into ezsystems:master Mar 12, 2018
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.

3 participants