cascading views doesn't work correctly for added application packages #805

Open
somomomo opened this Issue Dec 21, 2011 · 4 comments

Comments

Projects
None yet
6 participants

I wrote a more detailed post on StackOverflow about this (http://stackoverflow.com/questions/7759723/code-igniter-2-0-3-loader-add-package-path-view-cascading) but here's a quick rundown:

When adding an application package, if there is a naming collision in view files, setting the 2nd parameter in add_package_path to FALSE does not load the view file from the original environment and will still load the view file from the added package.

The bug is in the _ci_load function in the core Loader class. It starts at line 764 in the version 2.1.0 file where there's a check for the view file and the cascade value in the foreach loop. Since there's a check if the file exists first, the cascade value is irrelevant in determining whether the view file of the added package should be loaded. Thus, the cascade check does nothing if there is an actual conflict and the view file will always be loaded from the added package.

This bug has existed since at least 2.0.3 and I extended the_ci_load method in order to get around this. What I did was basically switch the order of the check for the view file and the cascade value so the cascade check is done first in the loop. Also, in the cascade check, I changed the break to be a continue. I'm actually thinking it may be better to put the check for cascade inside the beginning of the check for the view file but putting it before the view file check has worked for me.

Contributor

narfbg commented Jan 3, 2014

Honestly, I have no idea how this is supposed to work. Both the docs and the code is broken.

Contributor

mwhitneysdsu commented Jan 3, 2014

This portion of the code is not the most familiar area for me, and I don't have time to test it extensively at the moment and submit a pull request, but I would really expect that the cascade check would surround the break inside the file_exists check, i.e.:

            foreach ($this->_ci_view_paths as $view_file => $cascade)
            {
                if (file_exists($view_file.$_ci_file))
                {
                    $_ci_path = $view_file.$_ci_file;
                    $file_exists = TRUE;
                    if ( ! $cascade)
                    {
                        break;
                    }
                }
            }

Which, if I'm interpreting everything correctly, would imply that you would keep looping through the files if $cascade were true, allowing the view to load from a different path if available.

However, due to the order of the paths (according to @somomomo 's StackOverflow post linked above, since I haven't verified it), this would also result in the opposite behavior of what I would normally expect from cascading views, as it would override views from more recently-added paths with those from older paths.

This would imply that you would want to loop through the paths in the order in which they were added (in other words, in the opposite direction), though, in the default case where $cascade is true, this results in a slower exit from the loop (not to mention whatever overhead is involved in whatever method is chosen to reverse the loop direction).

As it stands, with the $cascade check outside of the file_exists check, it really seems more like the $cascade value is telling the loader "if you didn't find a view, yet, throw an error," rather than actually controlling cascading of the views.

@baypup baypup pushed a commit to baypup/CodeIgniter that referenced this issue Aug 20, 2015

@benedmunds benedmunds Merge pull request #805 from sparky672/patch-6
Update index.html
75484e6

ViVers commented Feb 9, 2016

mwhitneysdsu code is correct. I use this for my multiple application in one instance solution and it is working. Please add it to CI for everyone ;)

Xirt commented Jun 7, 2017

Noticed bug is still there after six years; fix in pull request #4444 is still working in current version though, but only requires modification of one lines and addition of another line (the other modifications in #4444 are merely lay-out changes to the code).

Only line to add is $_rev_view_paths = array_reverse($this->_ci_view_paths, true); just above the foreach-loop and refer the new variable $_rev_view_paths in the loop. Keep in mind that you need to do the same in MX_Loader in case you use the popular extension HMVC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment