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

Rendering stages get multiplied when viewDirs is an array #12977

Closed
memmaker opened this issue Jul 23, 2017 · 21 comments
Closed

Rendering stages get multiplied when viewDirs is an array #12977

memmaker opened this issue Jul 23, 2017 · 21 comments
Assignees
Labels
Projects

Comments

@memmaker
Copy link

@memmaker memmaker commented Jul 23, 2017

Expected and Actual Behavior

I am using a multi-module setup for Phalcon and each of my modules is supposed to have its own views in addition to the shared ones in the core module.

A few versions back, Phalcon introduced the ability to set the viewsDir of the view service to an array. This greatly simplifies my endeavor.

I am currently using this feature like this:

$view->setViewsDir(['../Modules/Core/Views/byController/', '../Modules/'.$moduleName.'/Views/']);

This works great, as Phalcon now looks in both of these directories for a view to render.

However I ran into problems with this setup. The rendering is now doubled for my menu and all my partials.

So my expectation would be, that I can just register multiple paths as viewDirs and Phalcon will use the first view it will find in any of these directories and use it for rendering. I expect each view to only be rendered once. I would also expect, that rendering of partials would be unaffected by setting the viewDirs to an array.

The result is, that partials get output once for each entry in the viewDirs variable. Which results in broken output.

I traced the problem down to the _engineRender() method in cphalcon/phalcon/mvc/view.zep. The problem is that we iterate over the viewDirs but do not check if we already rendered this view successfully. So we just do it again, if there are multiple viewDirs.

I even proposed a fix in this commit:
memmaker@b0fca22

I am currently not able to supply a minimal example. However you should be able to reproduce the Problem by setting the viewDirs of your view service to an array of paths and trying to render a partial.

We found the problem because we used a multi module setup and are setting the viewDirs of our view Service to an array in the modules.

I also found a way to workaround this bug by using the events manager.

Workaround:

        $this->menuHasBeenRendered = false;
        /** @var ManagerInterface $eventsManager */
        $eventsManager = $di->getShared('eventsmanager');

        // TODO: submit issue to phalcon for this bug..
        $eventsManager->attach(
            "view:beforeRenderView",
            function (Event $event, $component, $path)
            {
                if (ends_with($event->getData(), 'menu.phtml'))
                {
                    if ($this->menuHasBeenRendered)
                    {
                        return false;
                    }
                    $this->menuHasBeenRendered = true;
                }
                return true;
            }
        );

Details

  • Phalcon version: 3.1.2
  • PHP Version: 7.1.3
  • Operating System: linux
  • Installation type: Compiling from source
  • Zephir version (if any): n/a
  • Server: Nginx
  • Other related info (Database, table schema): mongoDb, Custom Schema
@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 23, 2017

Still exists in Phalcon Version: 3.2.1 compiled from Source

@sergeyklay

This comment has been minimized.

Copy link
Member

@sergeyklay sergeyklay commented Jul 24, 2017

Thank you for the report, and for helping us make Phalcon better. We'll try to sort out as soon as possible.

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 24, 2017

Update: I tried using the Version compiled from my "fix" and unfortunately it does not seem to solve the issue. The workaround still does solve it though..

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Jul 24, 2017

@memmaker are sure you used zephir to compile phalcon with your fix?

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 24, 2017

Huh? That might be a good point. I am not sure. I am using the default build process from source where I would call the 'install' script. Does this not use the Zephir Code?

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Jul 24, 2017

No, you need zephir to install extension from zephir code.

  1. git clone https://github.com/phalcon/zephir
  2. cd zephir
  3. ./install -c
  4. go to phalcon repo with your fix
  5. zephir build
@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 24, 2017

That's what I tried this time around. I first installed Zephir and then did this.

        && git clone --depth=1 git://github.com/memmaker/cphalcon.git \
        && cd cphalcon \
        && zephir build \
        && cd build \
        && ./install

I got a bunch of red warnings which I take to mean, that it was indeed compiling from source. Unfortunately I can still see no change at all when executing the code.

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Jul 24, 2017

Did you restart your webserver? No no.

        && cd build \
        && ./install

This is not needed, just zephir build alone. zephir build doesn't regenerate build folder, it uses ext folder, build folder and ext folder are separated things. zephir build already compiles extension and install it. Here you install extenssion two times - first from your new updated code, and second from original code, that's why you don't see changes.

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 24, 2017

I see, so I compiled it again with the old code..;) Thanks for your insights.

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 24, 2017

HOORAY! I can now confirm, that my proposed fix does indeed solve the issue.

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Jul 26, 2017

Cool, the question is if this is correct fix. I just think that we should set setViewsDir maybe something like assoc array like module => dir, and if there is certain key in this array for current module then look first for view in this path? If not look in others? Im just talking about order of paths in array, with your fix it will just look for first occurrence of view, and don't render anything more, but i think most of people would like more something like render view from current module, if not found go to others.

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 26, 2017

Hmm, well my fix does fix the issue I had. It is of course not a very elegant solution. But the question wether it is a valid fix depends on the intended semantics of setting viewsDir to multiple values.

I am not sure if I understand you correctly. My current solutions does exactly what you are talking about since that is my use case. I am looking for views in the current module viewDir and if there is none I want to render the default view. But before I added the check for hasBeenRendered the views got rendered twice. I can think of no use case where rendering the same view twice would be beneficial.

Nevertheless I would feel much more confident if someone who knows about the intended semantics of setViewsDir() had a look at this piece of code and could sort this out once and for all.

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Jul 26, 2017

Yea but if you will change order of paths you will have other view rendered if it exists :D What i talk about is we should first check in view path for current module i think(if it's set of course), then for other paths. Like:

$view->setViewsDir([
    'core' => '../Modules/Core/Views/byController/', 
    $moduleName => '../Modules/'.$moduleName.'/Views/'
]);

Let's assume that current module is user, and we want to render profile.volt, first we will check if key user exists in this array, if yes, use this view path, we found view - render it and stop, next we want to render data.volt, we again first check in module path, view is not found so we go to other paths.

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 26, 2017

Right. But that is nothing that worries me. It is like a priority order. First dir gets checked first. Also it works for the multi module setup.

Of course making the modulename explicit is more obvious but also less flexible. You can't have multiple overriding view dirs which are not in different modules. This would also not work if you are not using the module architecture at all..

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Jul 26, 2017

Yea, if you are not using module architecture then don't really see other way here. Just if current module is not empty then we should check first view dir for current module if it exists, not matter what order we provided in setViewsDir.

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 26, 2017

That is not for me to decide but for the maintainers of this project.

I have to thank anybody involved for the great piece of work. And I feel honoured if I should've been able to contribute to making it a bit better.

PS: Please don't kill my use case..;)

@Jurigag

This comment has been minimized.

Copy link
Member

@Jurigag Jurigag commented Jul 26, 2017

Well im just adding my opinion how it should work, not using multiple view dir anyway, didn't even know that there is such option.

@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jul 26, 2017

Yeah, they added it in a sneaky update and I guess it was more of a experimental feature since there are obviously some edge cases like this which are very hard to test for.

@davihu

This comment has been minimized.

Copy link

@davihu davihu commented Aug 29, 2017

Hey, have the same problem, partials are rendered twice. The fix can be done easy by replacing

https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/view.zep#L689

break; for return null; statement. I am using customized View class with this fix and work like a charm. When you use multiple view directories, it always renders the first template found under there dirs.

It works as I am familiar from other frameworks like Symfony. You can always handle the priorities in viewsDirs array.

@eva-thientran

This comment has been minimized.

Copy link

@eva-thientran eva-thientran commented Dec 11, 2017

@stale stale bot added the stale label Apr 16, 2018
@sergeyklay sergeyklay closed this Apr 16, 2018
@memmaker

This comment has been minimized.

Copy link
Author

@memmaker memmaker commented Jun 22, 2018

So, this is still not fixed in the upstream repo. Can we reopen this?

@niden niden added this to To do in 4.0 Release via automation Nov 11, 2019
@niden niden moved this from To do to Done in 4.0 Release Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.0 Release
  
Done
6 participants
You can’t perform that action at this time.