Refactoring Loader.php #5
Conversation
|
Tests passed successfully! |
|
Thanks for the pull request. 😊 I've been trying to find a moment to review this pull request today, but had continuous unexpected interruptions at home today, so haven't quite had the opportunity yet. Replying briefly now just to let you know that I've seen it. Hoping to look over it and review properly soon. 👍 |
src/Loader.php
Outdated
| * Method to sanitize the configurations | ||
| * @return void | ||
| */ | ||
| private function sanitizeConfigurations() : void |
There was a problem hiding this comment.
"sanitizeConfigurations": Moving that code to its own dedicated method isn't a bad idea, but I'm not sure about the choice of name for such a method, because the code isn't intended to "santise" anything, but rather, is intended to unpack or decode (depending on preferred terminology) bytes previously packed/encoded due to that the INI format either doesn't support those bytes natively, or doesn't support what they represent (e.g., \n for line-breaks) natively.
You'll notice that code block is somewhat mirrored by a similar code block at the updateConfiguration method, intended to repack/reencode configuration data when changes are made, in preparation for updating the INI file (assuming, of course, that the instance of an installation is using INI files in the first place, considering that both INI and YAML formats are supported by the package).
I've contemplated before whether to keep INI support, or whether to drop it entirely for an eventual future v4 release (because, although many different packages use it, and PHP itself has a php.ini configuration file, it still kind of really feels like a Windows things for me, and the idea of being truly OS-agnostic from a design standpoint has some conceptual and cosmetic appeal to me, though admittedly with very little technical incentive for it to be a priority), but a new major version release isn't likely to happen until a very long time into the future anyway, as I don't yet have enough breaking changes planned, or enough backwards-incompatible ideas in the works for releasing a new major version to be worthwhile just yet, so sticking with v3 for at least the foreseeable future (and thus retaining INI support for the foreseeable future).
src/Loader.php
Outdated
| } | ||
| } | ||
| } | ||
| unset($DirVal, $DirKey, $CatVal, $CatKey); |
There was a problem hiding this comment.
unset: Useful at that code block's original location (L306) as some early garbage cleanup, but becomes redundant when the code block is moved to its own dedicated method, as the variables in question exist only within that method's scope, and such variables and related memory will be cleaned up automatically by PHP anyway as soon as execution of the scope finishes.
There was a problem hiding this comment.
Can you please suggest me some resources (books,..etc) about how php engine handles memory and other things that end developer does not need to worry about, i am still beginner .Thanks for your time!
There was a problem hiding this comment.
I'll try to find some links I can share here. :-)
src/Loader.php
Outdated
| throw new \Exception('Vendor directory is undefined or unreadable.'); | ||
| } | ||
| } else { | ||
| if (! isset($_SERVER['DOCUMENT_ROOT'], $_SERVER['SCRIPT_NAME'])) { |
There was a problem hiding this comment.
The complaint from PHP-CS-Fixer:
1) src/Loader.php (no_trailing_whitespace, statement_indentation, return_type_declaration)
..can be fixed by dropping the space between the ! and the isset. A minor detail, but that's code-style. '^.^
| @@ -329,7 +315,7 @@ public function __construct( | |||
|
|
|||
| /** Calculate and build various paths. */ | |||
| foreach (['CachePath', 'QuarantinePath', 'SignaturesPath'] as $Path) { | |||
| if (!$$Path) { | |||
| if (!${$Path}) { | |||
There was a problem hiding this comment.
Not sure whether we need that change. Still executes fine either way.
There was a problem hiding this comment.
It is more readable i think
Hi
I was reading through the code and preferred to refactor the loader.php this way.
Hope this helps your project!
Thanks!