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

Use Absolute Paths #1579

Merged
merged 12 commits into from
Dec 15, 2018
Merged

Use Absolute Paths #1579

merged 12 commits into from
Dec 15, 2018

Conversation

natanfelles
Copy link
Contributor

@natanfelles natanfelles commented Dec 3, 2018

Description

Now is possible set the paths to anywhere.

For composer, normally is:

public $systemDirectory = FCPATH . '../vendor/codeigniter4/framework/system';

Tested moving the system to /usr/share/codeigniter:

public $systemDirectory = '/usr/share/codeigniter';

and, wow, really works! 😃

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@natanfelles
Copy link
Contributor Author

natanfelles commented Dec 3, 2018

What do you think about rename the FCPATH constant to PUBLICPATH?

I think it is more explanatory and more in line with the nomenclatures.

Maybe BASEPATH to CIPATH, or CODEIGNITERPATH, too.

Edit: BASEPATH to SYSTEMPATH, unless the system directory is renamed to codeigniter...

@jim-parry
Copy link
Contributor

I would be leary of changing the names of these base constants, as they have been in use for some time.

@natanfelles
Copy link
Contributor Author

@jim-parry BASEPATH files are not more compatible with CI3.

CI3 has the SELF, not used in CI4.

Front Controller Path is the Public Path, the PUBLICPATH....

@jim-parry
Copy link
Contributor

You are redefining all paths relative to the "public" folder,rather than the project root folder. I don't agree with this. The public facing content could be anywhere.
FCPATH is the folder containing index.php. I see FCPATH as the correct name for this, hence suiting the "nomenclature".
There is not SYSTEMPATH, but there is BASEPATH, and 375 references to that when I search in my IDE. I don't see a benefi to renaming it.

btw, in use for some time refers to the beginnings of CI4 3.5 years ago ... I wasn't thinking about CI3 specifically. Looking back at CI3, I see it has BASEPATH, FCPATH, APPPATH & VIEWPATH; so some of these do go back quite a ways!

We also have TESTPATH and SUPPORTPATH defined, as part of the unit testing bootstrap :-/

I don't see the benefit of renamig any of the existig path constants, but I can see there being a case for PUBLICPATH, even if it starts out the same as FCPATH (for now).

@natanfelles
Copy link
Contributor Author

An use case:

Some hostings, in this case, using ISPConfig has a private folder for user files, and web is like the public_html of cPanel.

In ISPConfig, CodeIgniter forces ROOTPATH to be one level above web, but the user do not have write permissions to put the application, system, etc, just above the web. He needs that the ROOTPATH is in ../private. What will be done when he set

$pathsPath = FCPATH . '../private/application/Config/Paths.php';

Like this, private is the ROOTPATH.

Of course, if he want the CodeIgniter public folder at [Document Root]/some/other the \Config\Paths must be updated to match the FCPATH.

With absolute paths, is also possible, although not advisable, set any path in "public" folders. But also exists .htaccess and other rules to protect access... This give total liberty to user put the folders where it wants.


There is not SYSTEMPATH, but there is BASEPATH, and 375 references to that when I search in my IDE.

SYSTEMPATH is the proposed name. IDE Replace in path solves it. Personally, I prefer CODEIGNITERPATH, and that system is renamed to codeigniter, because it holds the CodeIgniter namespace.

@jim-parry
Copy link
Contributor

@lonnieezell Hoping you will chime in with your wisdom here :)

@lonnieezell
Copy link
Member

To be honest, all of those paths and structure are the way the are simply to match the previous versions. They were some of the first things built in before I really had a good idea where the framework was going, so I have no issues with changing them. And, if we are going to do it, now is the time.

I like the idea of full paths, but they can become cumbersome at times, also, especially if your local and production environments are drastically different, though I suppose using the .env file and getenv() in the Paths config file would solve that.

I think it could be argued either way on whether to rename them. I think in CI3 FCPATH actually pointed to the index file, though it's been a while. If that's the case, it definitely made sense to call it the Front Controller Path.

I don't have large preferences on whether we keep it or rename it. I do think it makes sense to change BASEPATH to SYSTEMPATH to make it more clear what it actually points to. I have often renamed the system folder to codeigniter in different projects, but that could lead to some confusion, I think, because CodeIgniter isn't just the system folder to many people. I think they see it as the entire package, all of the folders, application, system, etc.

If we are renaming things, I think it makes sense to rename application to app, though. That would be more consistent with the constant, the namespace, and is generally nicer to use since less typing. I have been tempted to change that a number of times, but, especially when first starting the rewrite, kept it for nostalgia reasons.

@jim-parry
Copy link
Contributor

So, if I understand this correctly, the net effect of this PR is to...

  1. define the *Directory properties in Config/Paths to be relative to the application/Config/Paths.php file
  2. rename BASEPATH to SYSTEMPATH
  3. redefine ROOTPATH to be the folder above "application" instead of "public"
  4. drop the publicDirectory property
  5. Refer to ROOTPATH as the "main applicaiton directory", above the "application directory"

Lonnie has also suggested renaming the "application" folder to "app".

I am wondering why we have *Directory properties as well as *PATH constants. It seems to me that the constants would be simpler.
I agree that any changes along this line should be done sooner rather than later.

Re the next effect:

  • I have no problem with (2) and (4)
  • I have no problem with (1) conceptually , though it seems to have the same issue as defining paths relative to "public"
  • (3) should use what was defined in (1), which might mean constants instead; in fact, why not just go with the constants defined in bootstrap.php?
  • I disagree with (5) - it is confusing. I would call that the "project root" in English, with the "public" folder referred to as the "document root"; could be too long workihng with apache

@jim-parry
Copy link
Contributor

This is getting closer :) Looks like a few things to fix for travis-ci

What about the question of whether to stick with constants only, vs the *Directory properties? Would that not simplify things further?

This is a pretty big change for developers. Easy to adjust to, with search & replace, but it feels like it should be part of an alpha.4, together with directions for upgrading to it. The admin release scripts will likely need some tweaking to, regarding folder names.

@jim-parry jim-parry added this to the 4.0.0-alpha.4 milestone Dec 7, 2018
@lonnieezell
Copy link
Member

I agree this is best with Alpha 4.

As for constants - I don't think that should be the case. By us taking care of making the constants, etc, we have a chance to ensure that, at a minimum, a trailing slash is on the end of each path, which is assumed everywhere in the framework.

@titounnes
Copy link
Contributor

Great idea. we can more easily manage documents. we can run several environments with one "system"

@jim-parry
Copy link
Contributor

@natanfelles ohoh - Events conflict to resolve, sorry

@jim-parry jim-parry merged commit d586677 into codeigniter4:develop Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants