[Cache] FilesystemCommonTrait - Cannot use integers as namespace #23503
Comments
Although not explicitly documented, i think it actually needs to be a string by design. So passing an integer violates the contract.. making this a non-issue :) We may add constructor docblocks to clarify. |
|
Ok, that should be at least a test with is_string in the constructor to be sure, if the given parameter is a string or not. That will protect from passing all other types to the method. And as a developer uses an external library, he must not even know, how it works to use the given methods. And the only return values should be a value or an exception. In the current case, if someone will use a value that can come from database, url, xml, json, etc. and pass it directly - he won't be sure if the cache will be stored correctly - and now it is possible to overwrite the cache keys from another users - if the namespace will be an user id - as no sub-directory with the correct name will be created. This is not an expected behaviour - so for me - a bug :) Or do you follow the sentence: Our software never has bugs it just develops random features? :D |
|
You need to be sure it's a string (if that's the contract). If the constructor arg defines Asserting with
Software has bugs.. for sure; i wouldnt qualify this as such though :) |
|
I got your point - but this isn't the case ;) There is no parameter description in the constructor of the FilesystemAdapter class yet :) But I'll be then more cautious to use the other methods now :) Thanks for info :) |
|
That's why i suggested
;-) |
|
The interface (at least in 3.4): public function __construct($namespace = '', $defaultLifetime = 0, $directory = null);So I would say the type is most certainly defined as string here. |
This PR was merged into the 3.3 branch. Discussion ---------- [Cache] add constructor docblocks for clarity | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23503 | License | MIT | Doc PR | Commits ------- d1ce532 [Cache] add constructor docblocks for clarity
This PR was merged into the 3.2 branch. Discussion ---------- [Cache] add constructor docblocks for clarity | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23503 | License | MIT | Doc PR | Commits ------- 813a537 [Cache] add constructor docblocks for clarity
Hi,
I found that the method
Symfony\Component\Cache\Trait\FilesystemCommonTrait::init($namespace, $directory)has some constructs which prevent the namespacing.
The line 33: if (isset($namespace[0])) { ...
will work only if the given namespace is a string. If it'll be an integer - the isset will return false - and no sub-directory in cache root will be created!
Example:
results in:
First element:
bool(false)
but this notation:
returns the correct values:
First element: 8
bool(true)
I would recommend to change the isset directly to a preg_match called one line letter, declare the $match array and check for matching entry in it to throw the exception - or explicitly cast the incoming parameters to string ( it will be obsolete in PHP7 if the method would get typed parameters) .
A similar notation can be found also in the line 28, but there is a lower probability to get an integer instead of string. I know that the "isset" is the fastest method to check if a string exists - but in this case it is not the best solution.
Best regards,
Lukas
The text was updated successfully, but these errors were encountered: