Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Remove usage of JPATH_ROOT #305

Open
florianv opened this issue Dec 4, 2013 · 9 comments
Open

Remove usage of JPATH_ROOT #305

florianv opened this issue Dec 4, 2013 · 9 comments
Milestone

Comments

@florianv
Copy link
Contributor

florianv commented Dec 4, 2013

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

@mbabker
Copy link
Contributor

mbabker commented Dec 4, 2013

Is there a particular reason you suggest this?

@florianv
Copy link
Contributor Author

florianv commented Dec 4, 2013

Yes :

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

@florianv
Copy link
Contributor Author

florianv commented Dec 4, 2013

For example here :

$filename = JPATH_ROOT . "/language/overrides/$lang.override.ini";

(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
Copy link
Contributor

👍 It needs to be abstracted.

@piotr-cz
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

@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?

  • exists
  • getMetadata
  • getKnownLanguages

@dongilbert
Copy link
Contributor

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
Copy link
Contributor

Let's add the v2.0 Milestone then 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants