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

Added possibility to render view with short reference (added RelativeFallbackResolver). #6196

Closed
wants to merge 10 commits into from

Conversation

vnagara
Copy link
Contributor

@vnagara vnagara commented Apr 25, 2014

It makes add rendering view the same as php include file which contains
in the same folder.

Now possible use short form to look up in the same folder. If we are in view with template name 'very-long-view-top-name/very-long-view-top-name2/any-name' we can use:

$this->render('bar');   // like include 'bar.phtml';
// instead of
// $this->render('very-long-view-top-name/very-long-view-top-name2/bar');

should exist map resolver:

     'very-long-view-top-name/very-long-view-top-name2/bar' => 'foo/baz',

or the same path resolver.

It makes add rendering view the same as php include file which contains
in the same folder.
@Martin-P
Copy link
Contributor

You can also describe short names under the template_map configuration key: http://framework.zend.com/manual/2.3/en/modules/zend.view.quick-start.html#configuration

@vnagara
Copy link
Contributor Author

vnagara commented Apr 27, 2014

@Martin-P It is independent from template_map. Moreover getting view should be pointed inside 'template_map' or 'template_path_stack' as full view name 'very-long-view-top-name/very-long-view-top-name2/bar' (like full file path). We use short reference in the same view name space divided by '/' (like files in the same folder).

And It's better then include file as after moving main view into another folder subview will work as before,

@Ocramius Ocramius changed the title Added posibility to render view with short reference. Added possibility to render view with short reference. Apr 29, 2014
@Ocramius
Copy link
Member

@froschdesign I actually disagree on "no private methods" - instead of extending, people should implement the interface on their own.

@vnagara this needs tests to verify what you are doing.

@vnagara
Copy link
Contributor Author

vnagara commented Apr 29, 2014

@Ocramius I thank for reply.
I have written a test but it is ugly (as it should initiate plugin manager and view plugin that creates dependence of it) and I omitted it.
@froschdesign there were set private method as function do substitution of part code from function, You always can override entire function where this substitution is.

@Ocramius
Copy link
Member

@vnagara no test no merge :) We can discuss the quality of the test on the PR

@vnagara
Copy link
Contributor Author

vnagara commented Apr 29, 2014

I've finished and committed tests.

@Ocramius Ocramius self-assigned this Apr 29, 2014
@Ocramius
Copy link
Member

@vnagara the tests make your code much clearer now.

If I get this correctly, you basically want to use the current template as a name segment of the template that you want to render (for example in a partial)? I'm not sure if I agree with this approach...

@vnagara
Copy link
Contributor Author

vnagara commented Apr 30, 2014

Yeah, you got it right.
It annihilate wishes of people include php files from the same folder in view instead of rendering it. Additionally It give possibility don't care of view name with partial rendering.
And it works excellent (doesn't break code as it does including) even with moving/copying only one view file from one module to another.

I've been using (testing) such behaviour for half a year.

@localheinz
Copy link
Member

Please restart the build.

See #6205 (comment).

@vnagara
Copy link
Contributor Author

vnagara commented May 2, 2014

@Ocramius ping.

@Ocramius
Copy link
Member

Ocramius commented May 2, 2014

@vnagara I don't agree with it from an usability pov - it is quite confusing, and this logic (imo) should be part of a separate resolver that wraps around the aggregate resolver.

Thoughts?

@vnagara
Copy link
Contributor Author

vnagara commented May 3, 2014

@Ocramius If not for usability for what else should we write classes?
What there is confusing? We never use reference to view without slashes '/'. And It's not file path.

I like the idea to set it in the another class.
We should decide If such behaviour will be taken for further codding.

@Ocramius
Copy link
Member

Ocramius commented May 3, 2014

If not for usability for what else should we write classes?

Yeah, I agree with that, but I am questioning the usability here. Isn't it confusing that calling $this->partial('foo/bar', ['baz' => 'tab']) in a view will automatically lookup the/current/view/foo/bar? What if I wanted the absolute name to be used? It may be a bit confusing in cases where I wanted an exception (can't find a view), no?

I like the idea to set it in the another class.

I'm not yet sure how this would work, but it would likely be handled in the setter of the PhpRenderer in order to avoid creating a circular dependency between the PhpRenderer service and the ViewResolver service.

@vnagara
Copy link
Contributor Author

vnagara commented May 3, 2014

All test passed (pleas rerun travis).

What if I wanted the absolute name to be used?

There will be taken it if it exists, there is placed test for it (testReturnsResourceFromTopLevelIfExistsInsteadOfTheSameNameSpace). In my view it's is bad idea to create view name in top level without '/' (all them are created inside of module/vendor NS except layout and errors with his own NS). Any other absolute path will work.

I'm not yet sure how this would work, but it would likely be handled in the setter of the PhpRenderer in order to avoid creating a circular dependency between the PhpRenderer service and the ViewResolver service.

I avoided circular dependency in code. It tests only once and if name exists return it. It didn't add any value in any array. the/current/view/foo/bar will change in new partial view. Finally, I don't see possibility for any circular deps.

@Ocramius
Copy link
Member

Ocramius commented May 3, 2014

In my view it's is bad idea to create view name in top level without '/'

Yes, but the current resolver doesn't assume that there are slashes in the name of a view, while the new logic does that. Instead, just move it to a new resolver.

I don't see possibility for any circular deps.

Ah, yep, didn't notice that resolve() accepts the renderer as a parameter - that solves the problem. Moving this logic to a FallbackResolver or something like that would be quite easy.

@vnagara
Copy link
Contributor Author

vnagara commented May 3, 2014

Moving this logic to a FallbackResolver or something like that would be quite easy.

Yeah, It sounds good.
Should I put it in AggregateResolver as a parameter? If not where should be?

@Ocramius
Copy link
Member

Ocramius commented May 3, 2014

@vnagara yes, the wrapped resolver should be a constructor parameter:

class RelativeFallbackResolver implements ResolverInterface
{
    public function __construct(ResolverInterface $resolver)
    {
        $this->resolver = $resolver;
    }

    public function resolve(...)
    {
        if ($match = $this->resolver->resolve(...)) {
            return $match;
        }

        // your fallback logic goes here.
    }
}

@@ -27,8 +27,8 @@ class ViewResolverFactory implements FactoryInterface
public function createService(ServiceLocatorInterface $serviceLocator)
{
$resolver = new ViewResolver\AggregateResolver();
$resolver->attach($serviceLocator->get('ViewTemplateMapResolver'));
$resolver->attach($serviceLocator->get('ViewTemplatePathStack'));
$resolver->attach(new ViewResolver\RelativeFallbackResolver($serviceLocator->get('ViewTemplateMapResolver')));
Copy link
Member

Choose a reason for hiding this comment

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

You can simply wrap once around the aggregate resolver.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, how about $resolver->attach(new ViewResolver\RelativeFallbackResolver($resolver)) here? That would make the fallback resolver just one resolver with lower priority

Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoded instantiation of classes makes it impossible to overwrite them. I think a better way would be to retrieve the RelativeFallbackResolver from the ServiceLocator and thus making it possible to overwrite it when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I attached It. For avoiding wasted checks I did the FallbackResolver more clear and usable only with AggragateResolver.

@Martin-P It will be done by someone with moving the 'ViewResolver\AggregateResolver' there too. This are not included into this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be done by someone with moving the 'ViewResolver\AggregateResolver' there too. This are not included into this issue.

Can you point to the issue where this is done? Just to make sure someone is actually doing the moving. I don't see that any reference to this issue has been made yet.

freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
…c98907d

This class does not need to be designed for extension. Replacing it is preferrable.
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
…callable that was instantiated

Also moving success path to the end of the code block
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
freax pushed a commit to freax/zf2 that referenced this pull request Nov 27, 2014
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
…k/zendframework@c98907d

This class does not need to be designed for extension. Replacing it is preferrable.
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
…pose of the relative fallback resolver class
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
…e renderers that don't provide the correct plugin type
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
…ually use the callable that was instantiated

Also moving success path to the end of the code block
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
gianarb pushed a commit to zendframework/zend-view that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants