fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Cant replace/extend some core classes using package because of fuel::init mistake #512

Closed zaptree closed 13 years ago

zaptree commented 13 years ago

While trying to replace the core class Uri and use my own which is in a package I saw that the Autoloader was loading the original Uri class instead of mine. Investigating I realized that this happens because in the \Fuel\Core\Fuel::init method the Uri class gets used (line 151) before the actual bootstrap for all packages is called (Line 165) thus adding to the autoloader the new replacement too late.

When extending & replacing a class not under a package it works fine since the replacement class is added to the autoloader in the app/bootstrap.php before Fuel::init is even called meaning you can replace Uri, Config, Profiler and Fuel classes which you cant do if you want the replacements in a package.

A quick and easy solution which will at least remedy the Uri class is to move the code that adds packages to the beginning of the Fuel::init function (tested it and seemed to work fine). Of course though this solution will still not allow the replacement of the Config class which is used for getting the always_load.packages array and the Fuel class which is used to add_package and the init method is part of that class. So im guessing putting the code to load the packages and run their bootstrap should be put in the APP/bootstrap.php and done with a method that doesn't use the Config or Fuel classes would be the full solution.

WanWizard commented 13 years ago

This should not be the case.

The core class extensions or replacements that you have made and put in app/classes should be defined in the app/bootstrap.php.

Autoloader::add_classes(array(
    // Add classes you want to override here
    'Uri' => APPPATH.'classes/uri.php',
));

// Register the autoloader
Autoloader::register();

This happens before Fuel::init() is called so if you define your Uri class in the bootstrap, it will be used, and the Fuel core version will not be aliased to the global namespace. i.e. \Uri will load your version, \Fuel\Core\Uri will load the original version.

If the bootstrap entry is not present, \Uri will be aliased to \Fuel\Core\Uri, so both will call the same class.

zaptree commented 13 years ago

You didnt understand what I said. I know that extensions put in the app/classes work fine since the they are defined before the fuel::init class. But when putting a replacement in a package ie PACKAGE_NAME/classes/uri.php you define these in the bootstrap of the package PACKAGE_NAME/bootstrap.php and NOT in the app/bootstrap. This doesn't work because the package_bootstrap is called to late. I gave exact line numbers for it I hope I'm making my self clear. This is an actual issue not me making a mistake in configuration.

WanWizard commented 13 years ago

You want to override a core class from a package?

I'm afraid that's not possible for these few classes, as you'll have a chicken-and-egg problem. Fuel needs those classes to be able to access a package, therefore overloading them from a package is going to be very though,

But I'll reopen so the other devs can comment.

WanWizard commented 13 years ago

@dhorrigan - your thoughts on this?

zaptree commented 13 years ago

You are right about it being more difficult for the Fuel class and Config class since the actual code for adding packages relies on them but at least it can be fixed so that the Uri and Profiler class can be extended by putting the:

        foreach (\Config::get('always_load.packages', array()) as $package => $path)
        {
            is_string($package) and $path = array($package => $path);
            static::add_package($path);
        }

Code at the begining of the Fuel::init function right under the line:

\Config::load($config);

A full solution would be to do the including of the package/bootstrap inside the app/bootstrap.php before the Fuel::init is called without using the config class which is easy since the config classes options are are passed as a parameter in the Fuel::init method in that same file and later loaded into the config file. So your app/bootstrap would look like this:

 APPPATH.'classes/view.php',
));

$app_config=include(APPPATH.'config/config.php');
foreach ($app_config['always_load']['packages'] as $package => $path)
{   
    include (PKGPATH.$path.DS.'bootstrap.php');
}
// Register the autoloader
Autoloader::register();

// Initialize the framework with the config file.
Fuel::init($app_config);

/* End of file bootstrap.php */

And in the Fuel class you just comment out line 430 static::load($path.'bootstrap.php'); where it includes the package/bootsraps and you are good to go. I think that this solution should be fine (already tested it and everything seems to work good).

Of course there might be something I haven't taken into account since I am a fuelphp noob :D

jschreuder commented 13 years ago

This was talked over earlier, if some classes can be moved below the package loading we're very open to that but for now this was deemed an acceptable limitation for a few classes that need to be loaded before the always_load stuff is loaded.

Your solution is a bit messy (base config would be loaded twice to allow for this) and would make using class constants in config impossible (like the environment constants, which would throw errors).

If an elegant solution can be found we're all for it, but for now it's not yet possible.