app/AppKernel: store logs in _PS_ROOT_DIR_, not _PS_CORE_DIR_
#31720
Conversation
`_PS_CORE_DIR_` (i.e. `dirname(__DIR__)`, where the source code lives) is potentially read-only, while `_PS_ROOT_DIR_` should contain site-specific writable files.
|
Hello @MaxKellermann! This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community! |
|
Hi, thanks for this contribution! I found some issues with the Pull Request description:
Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much! About linked issuesPlease consider opening an issue before submitting a Pull Request:
(Note: this is an automated message, but answering it will reach a real human) |
| @@ -146,7 +146,7 @@ public function getCacheDir() | |||
| */ | |||
| public function getLogDir() | |||
| { | |||
| return dirname(__DIR__) . '/var/logs'; | |||
| return _PS_ROOT_DIR_ . '/var/logs'; | |||
There was a problem hiding this comment.
Actually, it shouldn't be based on these const which we should get rid of piece by piece but rather something more "Symfony friendly" like:
| return _PS_ROOT_DIR_ . '/var/logs'; | |
| return $this->getProjectDir() . '/var/logs'; |
And since it's actually what the method from the parent does this method should be removed
There was a problem hiding this comment.
No... getProjectDir() is relative to the source code:
Lines 210 to 213 in 21aa4c7
This would not change anything about this problem I'm trying to address - only add runtime overhead for the
realpath() call.
There was a problem hiding this comment.
But I still don't understand your need actually, currently the logs are store in /var/logs (relatively to the project root folder), just like cache files are stored in /var/cache The folders in /var are indeed supposed to be writable for the software to work
I don't think we can easily change the value of _PS_ROOT_DIR_ just to change the location of logs, because it is used in many other places and doesn't only impact the log folders. If this is really needed it would make more sense to think of a way to change this folder in another way but not relying on _PS_ROOT_DIR_ or _PS_CORE_DIR_ 🤔
There was a problem hiding this comment.
In my setup, the (global) directory where the PrestaShop sources are installed is mounted read-only; not only that, PHP processes have no write permissions there, even if it was mounted read-write.
Each PrestaShop instance then has a separate directory which is writable; there, uploads, logs, cached and other files shall be stored.
The idea is to have only one shared PrestaShop source tree for all instances which is never to be modified (until a new version gets installed).
There was a problem hiding this comment.
Using environment variables for this directory should be the answer here IMO. I think relying to a legacy prestashop constant is not here the way to go. That would require to have to think about a future proof solution to handle this, and not for this use case only.
WDYT ?
There was a problem hiding this comment.
OK, which one shall I use? Is there documentation?
I didn't see any documentation on _PS_ROOT_DIR_ and _PS_CORE_DIR_ (and therefore didn't know it's "legacy") - from the code I figured (guessed) the the former is for instance-specific (writable) data, and the latter is for the (read-only) sources, but it's very inconsistent.
There was a problem hiding this comment.
Yeah, but there is no documentation on this distinction read/write from previous teams, so it can be risky to assume this actually. Especially since _PS_ROOT_DIR_ and _PS_CORE_DIR_ have actually the same value, so nothing seems to indicate a difference in permissions
There was a problem hiding this comment.
Please help me understand what that means:
Yeah, but there is no documentation on this distinction read/write from previous teams, so it can be risky to assume this actually.
Does this mean that separation between sources and data is not possible with PrestaShop, and every PrestaShop instance must have a full copy of the source tree (38.055 files, 407 MB) intertwined with the data tree?
Especially since
_PS_ROOT_DIR_and_PS_CORE_DIR_have actually the same value, so nothing seems to indicate a difference in permissions
If they are designed to have the same value, what was the reason two variables were introduced and not one?
_PS_CORE_DIR_(i.e.dirname(__DIR__), where the source code lives) is potentially read-only, while_PS_ROOT_DIR_should contain site-specific writable files.