zendframework / zend-expressive-zendviewrenderer

zend-view PhpRenderer integration for Expressive
BSD 3-Clause "New" or "Revised" License
11 stars 12 forks source link

ZendViewRendererFactory override HelperPluginManager #54

Open popovserhii opened 6 years ago

popovserhii commented 6 years ago

I have Factory which retrieves PhpRenderer from Container and injects this to my AssetMiddleware which prepare js/css files and appends them to view helpers (headLink, headStyle, inlineScript, headScript). During this time PhpRenderer initialize HelperPluginManager

public function getHelperPluginManager()
{
    if (null === $this->__helpers) {
        $this->setHelperPluginManager(new HelperPluginManager(new ServiceManager()));
    }
    return $this->__helpers;
}

But when I inject \Zend\Expressive\ZendView\ZendViewRenderer to my Action \Zend\Expressive\ZendView\ZendViewRendererFactory override HelperPluginManager which was initialized before

// Inject helpers
$this->injectHelpers($renderer, $container);

Factory simply check if HelperPluginManager was registered in Container. There is no check if PhpRenderer already has initialized HelperPluginManager

private function retrieveHelperManager(ContainerInterface $container) : HelperPluginManager
{
    if ($container->has(HelperPluginManager::class)) {
        return $container->get(HelperPluginManager::class);
    }

    if (! $container instanceof InteropContainerInterface) {
        throw new Exception\InvalidContainerException(sprintf(
            '%s expects a %s instance to its constructor; however, your service'
            . ' container is an instance of %s, which does not implement that'
            . ' interface. Consider switching to zend-servicemanager for your'
            . ' container implementation if you wish to use the zend-view renderer.',
            HelperPluginManager::class,
            InteropContainerInterface::class,
            get_class($container)
        ));
    }

    return new HelperPluginManager($container);
}

This behavior creates two different HelperPluginManager and creates many problems.

geerteltink commented 6 years ago

Factory simply check if HelperPluginManager was registered in Container. There is no check if PhpRenderer already has initialized HelperPluginManager

If I understand you correctly, what you are writing here is correct behavior. The ZendViewRendererFactory does not check if the PhpRenderer has a HelperPluginManager registered.

I think what should be done in your custom PhpRendererFactory is grabbing the HelperPluginManager from the container and inject it into the renderer. This way your PhpRenderer and the ZendViewRenderer use the same HelperPluginManager.

class PhpRendererFactory
{
    public function __invoke(ContainerInterface $container) : PhpRenderer
    {
        $renderer = new PhpRenderer($config);

        if ($container->has(HelperPluginManager::class)) {
            $renderer->setHelperPluginManager(
                $container->get(HelperPluginManager::class)
            );
        }

        return $renderer;
    }
}
popovserhii commented 6 years ago

Yes, I did it but this behavior isn't obviously. I waste 3 hours for debugging this issue. I check the hash of PhpRenderer in several places and it was identical. And I didn't understand why HelperPluginManager in my MiddlewareFactory is different to HelperPluginManager in the view template.

class MiddlewareFactory implements FactoryInterface
{
    public function __invoke(ContainerInterface $container, $requestedName, array $options = null)
    {
        /** @var $asseticService AsseticService */
        $asseticService = $container->get('AsseticService');
        $renderer = ($container->has(PhpRenderer::class))
            ? $container->get(PhpRenderer::class)
            : new PhpRenderer();

        // this code was been added after debugging
        #if ($container->has(HelperPluginManager::class)) {
        #    $renderer->setHelperPluginManager($container->get(HelperPluginManager::class));
        #}

        return new AsseticMiddleware($asseticService, $renderer);
    }
}

If PhpRenderer has the possibility for default HelperPluginManager initialization, why I can't use this?

// vendor/zendframework/zend-view/src/Renderer/PhpRenderer.php:359
public function getHelperPluginManager()
{
    if (null === $this->__helpers) {
        $this->setHelperPluginManager(new HelperPluginManager(new ServiceManager()));
    }
    return $this->__helpers;
}

To my mind developer should not know internal logic. If he gets PhpRenderer from Container it should come with correct injected dependencies.

geerteltink commented 6 years ago

Your MiddlewareFactory is good. What you need is a PhpRendererFactory, as I described above. In that PhpRendererFactory you require the HelperPluginManager the same way as the ZendViewRendererFactory to make sure you have the same instance of the HelperPluginManager.

To my mind developer should not know internal logic. If he gets PhpRenderer from Container it should come with correct injected dependencies.

In general you are right. However there is no PhpRendererFactory so this is something you need to create yourself. It's more of an edge case as not many developers use it this way. But I can see your point. I don't use the zend-view renderer myself so others need to decide if this a useful addition. You can always create a PR and add the missing PhpRendererFactory.

popovserhii commented 6 years ago
  1. Yes, you are right there isn't any PhpRendererFactory and I simply register it as invokables

    'dependencies' => [
    'invokables' => [
        PhpRenderer::class => PhpRenderer::class,
    ],
    ]
  2. You can always create a PR and add the missing PhpRendererFactory

I clearly understood you, you suggest me create PhpRendererFactory and pull to a repository. Which one?

  1. ZendViewRendererFactory already check if HelperPluginManager was initialized in Container, it would be good check if it was already injected to PhpRenderer and doesn't override existed.
weierophinney commented 4 years ago

This repository has been closed and moved to mezzio/mezzio-laminasviewrenderer; a new issue has been opened at https://github.com/mezzio/mezzio-laminasviewrenderer/issues/2.