Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Fixed missing root using layout viewhelper #62

Merged
merged 6 commits into from Jan 14, 2019

Conversation

fduarte42
Copy link
Contributor

Fixed bug mentioned here:
zendframework/zend-expressive-skeleton#68

@@ -269,6 +269,11 @@ private function injectNamespacedResolver(AggregateResolver $aggregate) : void
$aggregate->attach(new NamespacedPathStackResolver(), 0);
}

/**
* @param AggregateResolver $aggregate
Copy link
Member

Choose a reason for hiding this comment

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

Please undo all these changes as they are not needed. The method declaration already contains this information!
Additionally the topic of this PR the fix #68 rather than adding more DocBlocks or clean up anything.

That also makes the review easier. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I reverted the cleanup.

@fduarte42
Copy link
Contributor Author

Everything should be clean now. Please let me know if it is ok. We need this fix for a deployment.

Thanks in advance.

@froschdesign
Copy link
Member

@fduarte42

We need this fix for a deployment.

Then use your own fork in the meantime: https://getcomposer.org/doc/04-schema.md#repositories

@fduarte42
Copy link
Contributor Author

ok, we can use this for workaround now. But just out of curiosity: How long does such a review take?
No pressure though. Really.

@geerteltink
Copy link
Member

ok, we can use this for workaround now. But just out of curiosity: How long does such a review take?
No pressure though. Really.

Depends on the PR. I don't use zend-view myself so I won't merge this as I have no clue what it does and if this would be a good solution. You would have to wait for someone else and since it's Friday it will probably be early next week.

$result = $this->renderModel($child, $renderer);

if ($child !== $root) {
$viewModelHelper = $renderer->plugin('view_model');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be $renderer->plugin(ViewModel::class); where ViewModel is aliased to \Zend\View\Helper\ViewModel. At some point, all those old string aliases will go away won't they?

Copy link
Contributor Author

@fduarte42 fduarte42 Jan 11, 2019

Choose a reason for hiding this comment

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

That ist not correct. The ViewHelper is called 'view_model'. It is not using the container here, but the HelperPluginManager. You could call it with $renderer->view_model() also. But I prefer to call it this way. That's the way Zend/View does it internally.

Here is the code in PhpRenderer:

            // Give view model awareness via ViewModel helper
            $helper = $this->plugin('view_model');
            $helper->setCurrent($model);

Or in Layout helper:

    protected function getViewModelHelper()
    {
        if (null === $this->viewModelHelper) {
            $this->viewModelHelper = $this->getView()->plugin('view_model');
        }

        return $this->viewModelHelper;
    }

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. I tried to use the class, but it does not work.
Perhaps we need to pump up the dependency of zend view to a higher version.
I have to investigaste it further.
And you are right. The HelperPluginManager is a Container. My bad.

Copy link
Contributor Author

@fduarte42 fduarte42 Jan 11, 2019

Choose a reason for hiding this comment

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

I use the class now. It is working now. I did import the worng class initially. Thanks for correcting me. This is much better.

@weierophinney weierophinney merged commit 5b628a0 into zendframework:master Jan 14, 2019
weierophinney added a commit that referenced this pull request Jan 14, 2019
Fixed missing root using layout viewhelper
weierophinney added a commit that referenced this pull request Jan 14, 2019
weierophinney added a commit that referenced this pull request Jan 14, 2019
weierophinney added a commit that referenced this pull request Jan 14, 2019
Forward port #62

Conflicts:
	CHANGELOG.md
@weierophinney
Copy link
Member

Thanks, @fduarte42!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants