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

App\Config\Routes loaded twice #2203

Closed
maddrid opened this issue Sep 9, 2019 · 32 comments · Fixed by #5780
Closed

App\Config\Routes loaded twice #2203

maddrid opened this issue Sep 9, 2019 · 32 comments · Fixed by #5780
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@maddrid
Copy link

maddrid commented Sep 9, 2019

Describe the bug
App\Config\Routes file is loaded twice

CodeIgniter 4 version
All -Latest

Affected class
RouteCollection.
Codeigniter
Codeigniter->run () ---> handleRequest() --->tryToRouteIt () = require APPPATH .'Config/Routes.php';

** steps to reproduce **
Add a function to App\Config\Routes file

@lonnieezell
Copy link
Member

Just did a quick scan of the codebase and it only appears to be loaded once. system/Config and any environment-specific files are also loaded.

The only way I can think of this happening otherwise is when it scans any namespaces for additional routes files it's not ignoring those two.

I'll try to look into this in the next few days.

@lonnieezell lonnieezell added this to the 4.0.0-rc.2 milestone Sep 11, 2019
@jim-parry jim-parry modified the milestones: 4.0.0-rc.2, 4.0.0-rc.3 Sep 24, 2019
@lonnieezell
Copy link
Member

Cannot reproduce. Added:

function testme() { echo 'etest'; }

Into my routes file. Fired up the server (php spark serve) and loaded the page. No complaints about function already defined.

@maddrid
Copy link
Author

maddrid commented Oct 9, 2019

Capture
just tested

CRITICAL - 2019-10-09 15:10:45 --> Cannot redeclare Config\test() (previously declared in N:\a\htdocs\ci\app\Config\Routes.php:78)
#0 [internal function]: CodeIgniter\Debug\Exceptions->shutdownHandler()
#1 {main}

@lonnieezell
Copy link
Member

Is this on a clean install? You don't maybe have a helper being loaded with a similarly named function do you?

@maddrid
Copy link
Author

maddrid commented Oct 10, 2019

Yes clean install . Just downloaded the latest version from here.
Tested again with various function names .
Also , the routes can be counted .
total routes = routes x 2

@lonnieezell
Copy link
Member

@web-media what happens if you comment out the toolbar in app/Config/Filters.php?

@maddrid
Copy link
Author

maddrid commented Oct 10, 2019

Commented the toolbar .

public $globals = [
		'before' => [
			//'honeypot'
			// 'csrf',
		],
		'after'  => [
		//	'toolbar',
			//'honeypot'
		],
	];

Works fine if no function in routes.php.
Error page if function added .
Capture

@MGatner
Copy link
Member

MGatner commented Oct 10, 2019

Can you change it to this to get a little more helpful feedback on the what and when:

$routes->get('/', 'Home::index');

trace();
if (function_exists('cvvvv'))
{
	dd($routes);
}
function cvvvv() {'dsd';}

@maddrid
Copy link
Author

maddrid commented Oct 10, 2019

<ROOT>/ci/app/Config/Routes.php:76 trace()

⇄→<ROOT>/ci/system/CodeIgniter.php:709 require()

⇄→<ROOT>/ci/system/CodeIgniter.php:295 CodeIgniter\CodeIgniter->tryToRouteIt(CodeIgniter\Router\RouteCollectionInterface $routes = null)

⇄→<ROOT>/ci/system/CodeIgniter.php:245 CodeIgniter\CodeIgniter->handleRequest(CodeIgniter\Router\RouteCollectionInterface $routes = null, $cacheConfig, bool $returnResponse = false)

⇄→<ROOT>/ci/public/index.php:45 CodeIgniter\CodeIgniter->run(CodeIgniter\Router\RouteCollectionInterface $routes = null, bool $returnResponse = false)

→ Called from /ci/app/Config/Routes.php:76

Capture

@MGatner
Copy link
Member

MGatner commented Oct 10, 2019

Can you run it from CLI and put the output in PasteBin (or anywhere easier to read)?

php spark routes

@MGatner
Copy link
Member

MGatner commented Oct 10, 2019

Initial reactions have me weirded out. I only see one Trace call in your screenshot when there should have been two, and unless I'm misunderstanding the parser I think that dd() should have been called before the second declaration of the function, which was not the case.

@maddrid
Copy link
Author

maddrid commented Oct 10, 2019

The only thing that came to my mind is that RouteCollection method discoverRoutes doesn't do the job somehow in this lines

if ($file === APPPATH . 'Config/Routes.php')
				{
					continue;
				}
protected function discoverRoutes()
	{
		if ($this->didDiscover)
		{
			return;
		}

		// We need this var in local scope
		// so route files can access it.
		$routes = $this;

		if ($this->moduleConfig->shouldDiscover('routes'))
		{
			$files = $this->fileLocator->search('Config/Routes.php');

			foreach ($files as $file)
			{
				// Don't include our main file again...
				if ($file === APPPATH . 'Config/Routes.php')
				{
					continue;
				}

				include $file;
			}
		}

		$this->didDiscover = true;
	}

@lonnieezell
Copy link
Member

lonnieezell commented Oct 10, 2019

Can we get some stats from you? I just tried again with a fresh clone, adding a function to app\Config\Routes.php, started up php spark serve and loaded the site. It loaded without throwing errors. So might be system-dependent thing.

Could you provide:

  • OS type/version
  • how you're serving the page (if a server other than the built-in one, please provide type and version)
  • PHP version
  • uri protocol (from app\Config\App.php)

And any other information you can think to provide.

UPdate:

Realized I was in production mode when I tried that. So copied env to .env, set the baseUrl, and set environment to production. Still worked for me.

For reference, I'm on Mac 10.4.5, running PHP 7.2.7, using CI's built-in server, and REQUEST_URI as the protocol.

@lonnieezell lonnieezell reopened this Oct 10, 2019
@maddrid
Copy link
Author

maddrid commented Oct 10, 2019

XAMPP for Windows 7.3.7
Apache/2.4.39 (Win64) OpenSSL/1.1.1c PHP/7.3.7
Windows NT PC 6.1 build 7601 (Windows 7 Ultimate Edition Service Pack 1) AMD64
PHP Version 7.3.7
and REQUEST_URI as the protocol.

Commenting this line //include $file;
the routes are loaded once ...
Seems that the bug is here

if ($file === APPPATH . 'Config/Routes.php')
				{
					continue;
				}
protected function discoverRoutes()
	{
		if ($this->didDiscover)
		{
			return;
		}

		// We need this var in local scope
		// so route files can access it.
		$routes = $this;

		if ($this->moduleConfig->shouldDiscover('routes'))
		{
			$files = $this->fileLocator->search('Config/Routes.php');

			foreach ($files as $file)
			{
				// Don't include our main file again...
				if ($file === APPPATH . 'Config/Routes.php')
				{
					continue;
				}

				//include $file;
			}
		}

		$this->didDiscover = true;
	}

@maddrid
Copy link
Author

maddrid commented Oct 10, 2019

$file is

N:\a\htdocs\ci\system/\Config/Routes.php
N:\a\htdocs\ci\app\/\Config/Routes.php"

is compared to

N:\a\htdocs\ci\app\Config/Routes.php

APPPATH
it's a path issue on windows

@lonnieezell
Copy link
Member

Awesome. At least we can pinpoint what needs work. Thanks for sticking with it and helping us locate that!

@lonnieezell
Copy link
Member

If you happen to create a PR with a fix, that'd be awesome. Might be a few days until I have time to really dig in, especially with needing to update my Windows install and get it working for this again.

@maddrid
Copy link
Author

maddrid commented Oct 10, 2019

you're welcome ! always glad to help .
Not knowing yet ci4 well enough to dig in deep . I'm barely scratch the surface :)

@lonnieezell
Copy link
Member

No better way to learn :D

lonnieezell added a commit that referenced this issue Oct 16, 2019
Correct cleaning of namespaces in FileLocater for better Windows compatibility. See #2203
@lonnieezell
Copy link
Member

@web-media Just merged a fix for that. Please pull down the latest and let me know if it works for you.

@maddrid
Copy link
Author

maddrid commented Oct 16, 2019

Solved .

@maddrid maddrid closed this as completed Oct 16, 2019
@zsy78191
Copy link

zsy78191 commented May 20, 2021

I solved it by changing
if ($file === APPPATH . 'Config/Routes.php')
to
if (realpath($file) === APPPATH . 'Config/Routes.php')

@livelogic
Copy link

I'm using version 4.1.3 and noticed the same issue.
In my case, the solution posted by maddrid works - commenting the line in the RouteCollection module:
//include $file;
Tampering with CI system files is not my favourite approach, and i'm wondering if this is really a glitch in CI libraries or rather something else i'm missing.

@livelogic
Copy link

Still happening to me with 4.1.8
Again i can fix it with the workaround suggested from Maddrid
//include $file;
CI 4.1.8, PHP 8.1.3, Windows 10, IISExpress

@kenjis
Copy link
Member

kenjis commented Mar 5, 2022

@livelogic What value is in the $file?

if ($file === APPPATH . 'Config/Routes.php') {

@livelogic
Copy link

livelogic commented Mar 6, 2022

@kenjis: yes, that seem to be the culprit.

BTW, upgraded to 4.1.9 but nothing is different regarding this issue.
Two files are returned by the fileLocator->search method:

  • "(project root)\app\Config\Routes.php"
  • "(project root)\system\Config\Routes.php"

These paths have backslashes as directory separators, as expected in Windows environment, but the culprit line compares a path that has a Linux pattern appended:

if ($file === APPPATH . 'Config/Routes.php') {

So, instead of commenting the line:

include $file;

i tried this:

if ($file === APPPATH . 'Config\\Routes.php') {

and things work fine (app\Config\Routes.php is loaded only once).
I haven't tried a deeper debugging but it seems clear that the paths should be normalized before comparison.

I also notice another strange thing in the same method: it loads unconditionally the system\Config\Routes.php file via include, although this file has already been loaded by app\Config\Routes.php, via require:

if (file_exists(SYSTEMPATH . 'Config/Routes.php')) {
	require SYSTEMPATH . 'Config/Routes.php';
}

Double-loading this file doesn't seem harmful, still it doesn't seem like intended behavior.

@kenjis
Copy link
Member

kenjis commented Mar 6, 2022

@livelogic Thank you for the info.
Please try #5780

@livelogic
Copy link

@kenjis Tested the modified code in @5780, and it works fine on Windows: the Routes.php files in both app\ and system\ branches are loaded only once now.

@kenjis
Copy link
Member

kenjis commented Mar 8, 2022

@livelogic Thank you for confirming.
The fix was merged into develop branch, and it will be included in v4.2.0.

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label May 6, 2022
@pixobit
Copy link
Contributor

pixobit commented Sep 24, 2023

Is this still an issue? Ran into it on v4.4.1, and it seems to be the same behavior. If I comment out include $file; then it works...

@kenjis
Copy link
Member

kenjis commented Sep 24, 2023

If you use Windows, it may be another bug. See #7930

@pixobit
Copy link
Contributor

pixobit commented Sep 24, 2023

Yes, can confirm the DIRECTORY_SEPARATOR solves this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants