APPPATH, SYSPATH and similar constants. Rename? #2
Comments
|
Yeah, a bit early. See the readme :) I've given some thought to a compatibility layer (unofficially) before, too, and I'm not convinced that this needs to be different. And it seems like it might help in some ways to keep them the same, because if the structure is kept with application and system folders, then anytime you use those paths in your application, they still point to the same general area. If you're setting up multiple applications that share the system folder, then your old application folder could use the same constants and still be correct. I'm not 100% convinced this change is necessary, but it's definitely something to consider as we move forward. |
|
Further thoughts on this? If a change is to happen, then now (before first milestone) would be the time to do it. |
|
I agree - not in favor of this one, in large part due to the familiarity aspect. While he does have a point for specific upgrade paths, I see upgrading from 3 to 4 as being a large enough of a project as to not necessarily be feasible. However, you could have both versions running side by side, and certain modules run on one version and another run on another version, the main tricky thing here would be handling routing, but should be possible. |
|
I am not convinced that we should provide for CI3 and CI4 side-by-side (in the same app, I presume) ... that feels like way too big a can of worms. I don't recall any attempt to do that with CI2 -> CI3, it was one or the other IIRC. From what I am seeing, if the constants are extensively used in code (sounds like a good practice), then a conversion/migration guide, which referenced them, would be a better service than renaming the path constants. |
|
OH, yeah, I wasn't saying that we go out of our way to allow to run side by side, but that's how any large migrations are likely to work - moving one part of the site at a time over. I've heard people on the forums talk about doing that between 2 and 3. The constants didn't provide any problems there, but the codebases were similar enough that it probably wouldn't have caused a problem. |
|
A suggestion without thinking too much: |
|
I already suggested in other discussions that Path constants is OLD PHP practice and perhaps it must be replaced, either by path generator class or something like that. Specially with the new autoloader its even more comfortable to have some class to manage paths instead of some inline written paths using constants.. By using constants at each place where you use them you have to always check if the required file exists and then or else.. |
|
Hmmm ... define CI4 constants for the path name (CI4_APP, CI4_FC, ...) and then have something like path(CI4_APP)? Or CodeIgniter\Path class with app, fc, view, etc methods? |
|
I see no problem with global constants for these type of things, personally. I know people like to have classes for everything and shy away from any "old school" PHP, but they all have their place. Besides, a class takes more CPU and memory to use than a constant. And having a new class just to hold static values is the type of thing I was always told was bad use of OOP. These are values that will never change throughout the request so a constant is the language feature intended to handle those. While we could add it to the Codeigniter class, and I'd be semi-ok with that, the use is even more complex for developers because it's no longer It's getting a reference to the CodeIgniter class and then calling a method on that class to return a value that will never change. Seems overkill to me. Personally, the language features are still there and it's not bad form to use standalone methods, or global constants, etc, as long as they're used with care. Spaghetti code can be created in thoroughly OOP code (though I understand the term for that is lasagna code), so it's not that the feature is used, it's how it's used. And we are trying to keep some of the flavor and feel of CI after all. :) I also don't see a need to "namespace" the constants. It might make sense if we didn't have 10+ years of history with the old ones. As far as I can think of for the upgrade process, you're only ever going to be in one version or the other during a single request, so I can't see the constants interfering with each other. And, if they did, the system would crash, telling the dev what to fix. While I'll go along with the majority on this, neither of these suggestions seem necessary or even like a great idea. |
|
I vote that we stick with the existing constants (their names). Migrating is going to be enough fun without changing them. |
|
Lets keep it as implemented. |
Views hints - Use XPath search & renaming for style guide
Filter in the router supports after. Fixes #1315
Match delete before update in websafe resource options, fixes /resour…
newest version.
CRITICAL - 2019-12-10 19:17:06 --> Argument 2 passed to dot_array_search() must be of the type array, null given, called in /home/zdamy/site/system/Session/Session.php on line 500
#0 /home/zdamy/site/system/Session/Session.php(500): dot_array_search('ausItem', NULL)
codeigniter4#1 /home/zdamy/site/system/Common.php(347): CodeIgniter\Session\Session->get('ausItem')
codeigniter4#2 /home/zdamy/site/app/Models/AdminUserModel.php(87): session('ausItem')
BTW/ I am not sure if that method should not look like:
```
public function get(string $key = null)
{
if (! empty($key))
{
if(! is_null($value = dot_array_search($key, $_SESSION??[])))
{
return $value;
}
return null;
}
elseif (empty($_SESSION))
{
return [];
}
$userdata = [];
$_exclude = array_merge(
['__ci_vars'], $this->getFlashKeys(), $this->getTempKeys()
);
$keys = array_keys($_SESSION);
foreach ($keys as $key)
{
if (! in_array($key, $_exclude, true))
{
$userdata[$key] = $_SESSION[$key];
}
}
return $userdata;
}
```
[20-Aug-2020 23:13:08 America/New_York] PHP Fatal error: Uncaught Error: Call to undefined function CodeIgniter\locale_set_default() in /home/AppCloudData/public_html/system/CodeIgniter.php:184 Stack trace: #0 /home/AppCloudData/public_html/system/bootstrap.php(181): CodeIgniter\CodeIgniter->initialize() codeigniter4#1 /home/AppCloudData/public_html/public/index.php(36): require('/home/AppCloudData/...') codeigniter4#2 {main} thrown in /home/AppCloudData/public_html/system/CodeIgniter.php on line 184
NumberFormatter::setAttribute(): Passing null to parameter #2 ($value) of type int|float is deprecated
ErrorException DateTime::createFromFormat(): Passing null to parameter #2 ($datetime) of type string is deprecated
Passing null to parameter codeigniter4#2 ($string) of type string is deprecated
ErrorException: NumberFormatter::setAttribute(): Passing null to parameter #2 ($value) of type int|float is deprecated
1) CodeIgniter\Database\DatabaseTestCase\DatabaseTestCaseMigrationOnce1Test::testMigrationDone TypeError: json_encode(): Argument #2 ($flags) must be of type int, bool given /home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/Constraints/SeeInDatabase.php:118 /home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:282 /home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php:86
Sorry for barking so early. :-)
Sample APPPATH, SYSPATH usage:
https://github.com/lonnieezell/CodeIgniter4/blob/develop/application/config/autoload.php#L33
Is it possible these constants to be renamed or name-spaced? Maybe in the distant future I'll make an attempt for compatibility layer about CI3. A name collision between certain CI3 and CI4 constants seems to me an obstacle for this goal.
The text was updated successfully, but these errors were encountered: