joomla / joomla-framework

[READ-ONLY] This repo is no longer in active development. Please see https://github.com/joomla-framework for the individual Framework packages.
http://framework.joomla.org
GNU General Public License v2.0
189 stars 140 forks source link

Remove usage of JPATH_ROOT #305

Open florianv opened 10 years ago

florianv commented 10 years ago

https://github.com/joomla/joomla-framework/search?q=JPATH_ROOT&ref=cmdform

mbabker commented 10 years ago

Is there a particular reason you suggest this?

florianv commented 10 years ago

Yes :

1) It adds a dependency, people need to define it 2) Absolute file paths cannot be passed everywhere

florianv commented 10 years ago

For example here : https://github.com/joomla/joomla-framework/blob/436253110c9e47c8cbfb9a6b2780e812480a31ab/src/Joomla/Language/Language.php#L197

(Well it's not a good example because the path is hardcoded) but you would need to change the constant just before calling the constructor to adjust your path if you want an other location for your overrides. You cannot use two classes with different JPATH_ROOT values.

Also forcing people to create a constant to use the framework will make a lot of them run away before even trying to use it.

eddieajau commented 10 years ago

:+1: It needs to be abstracted.

piotr-cz commented 10 years ago

@florianv I've been thinking about same solution for the Language package. What about

private $basePath;

/**
 * @param   string  $basePath  Location of language files
 */
public function __construct($lang = null, $debug = false, $basePath = null)
{
    if (is_null($basePath))
    {
        throw \InvalidArgumentException('Provide location of language files.');
    }

    $this->basePath = $basePath;
}

As the Framework follows Semver, does it mean that this change in the API should go to 2.0?

I'd like to add some kind of default value for $basePath, but as we'd like to get rid of constants, I can't think of anything else than __DIR__ but this doesn't make much sense.

Other solution would be to move the code that's using JPATH_ROOT in constructor to another method, ie. public function initialize($basePath) and invoke it if $basePath is provided in constructor.

Actually I'm not sure if we follow Semver yet, @eddieajau ?

florianv commented 10 years ago

@piotr-cz I think it would be better to pass directly the full path to the directory where we can find language files / overrides

piotr-cz commented 10 years ago

@florianv for sure.

Still there are few static methods that use the constant. Since it would be property of the instance, how do you think we should handle $basePath there? Use a static property or have an argument in each of these methods?

dongilbert commented 10 years ago

The Language package needs to be refactored to be not just a bunch of static methods, but that's going to have to wait until v2, along with the removal of JPATH_ROOT. Getting it in before then would break BC. Don't worry though, I see 2.0 coming maybe in 6-8 months time.

piotr-cz commented 10 years ago

Let's add the v2.0 Milestone then :smile: