Moved the tmp/ path into the config (was hardcoded everywhere)
#6658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mnapoli
added
the
c: Platform
The `tmp/` path was hardcoded everywhere, which resulted in using `SettingsPiwik::rewriteTmpPathWithInstanceId()` to rewrite it for specific use cases. I've moved that path into the config, and replaced all hardcoded usage (and calls to `rewriteTmpPathWithInstanceId()`) by a `get()` from the container. Getting entries from the container is a bad practice and dependency injection should be preferred, but we do baby steps. When refactoring those classes to DI, we'll replace calls to the container with proper dependency injection. Another thing we'll need to do too is move the hardcoded *sub-path* of `tmp/` (e.g. `tmp/sessions/`) into the config also (but again: baby steps). Another future step would be to remove completely instance ID and let it be handled by a plugin (or by end-user config). Having the `tmp/` path in the config means that plugins or users can override it and know it will be taken into account everywhere in Piwik.
Conflicts: core/Session.php
Conflicts: tests/PHPUnit/Integration/LogTest.php
Conflicts: plugins/Installation/SystemCheck.php
|
Hi @mnapoli if this is for 2.10.0 could you set the milestone? cheers |
|
Done. Ready to be merged. |
| @@ -132,10 +133,7 @@ public static function start($options = false) | |||
| $e->getMessage() | |||
| ); | |||
|
|
|||
| $ex = new MissingFilePermissionException($message, $e->getCode(), $e); | |||
| $ex->setIsHtmlMessage(); | |||
There was a problem hiding this comment.
is this change expected in the PR?
There was a problem hiding this comment.
Nope, fixed. Waiting for the tests to pass before merging.
|
Do not merge yet ;) |
|
All good, @mattab please review 4a2145a#diff-842c76803d19071217437497af04afd5R327 it was necessary to check for the config file before testing for the permissions on the directories. The reason is that the directories are now resolved by getting the |
mattab
pushed a commit
that referenced
this pull request
Dec 2, 2014
Moved the `tmp/` path into the config (was hardcoded everywhere)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based on #6648
The
tmp/path was hardcoded everywhere, which resulted in usingSettingsPiwik::rewriteTmpPathWithInstanceId()to rewrite it for specific use cases. This is risky as developers have to remind to call this "rewrite" method everytime (and in some place it wasn't…), and it makes everything not really customizable (rewriteTmpPathWithInstanceId()had to be added so that for "multiple piwik instances scenarios" we could customize the path with an "instance ID"…).I've moved the
tmp/path into the config, and replaced all hardcoded usage (and calls torewriteTmpPathWithInstanceId()) by aget()from the container.Getting entries from the container is a bad practice and dependency injection should be preferred, but we do baby steps. When refactoring those classes to DI, we'll replace calls to the container with proper dependency injection.
Another thing we'll need to do too is move the hardcoded sub-path of
tmp/(e.g.tmp/sessions/) into the config also (but again: baby steps).Another future step might be to remove completely instance ID and let it be handled by a plugin (or by end-user config). Having the
tmp/path in the config means that plugins or users can override it and be assured that it will be taken into account everywhere in Piwik.