Class BaseStringHelper: optimize & bugfix
#18799
Conversation
- Optimize `BaseStringHelper::basename()`: passing normalized path to PHP `basename()` - Bugfix `BaseStringHelper::dirname()`: - if path '/dir/sub-dir/', method return '/dir' instead '/dir/sub-dir' - if path '/dir', method return '/' instead ''
|
|
||
| return $path; | ||
| return \basename($path); |
There was a problem hiding this comment.
This seems unnecessary and even wrong since basename is local aware.
There was a problem hiding this comment.
@bizley fixed
This seems unnecessary and even wrong
"seems"? maybe you test it before? it's not true and work faster - php find function in local namespace and only after it in global namespace
local aware
you mean global? (:
There was a problem hiding this comment.
I'm referring to the "Caution" message stated at https://www.php.net/manual/en/function.basename.php
There was a problem hiding this comment.
@bizley it's stupid:
- we don't pass encoding to method
- encoding in
mb_*functions: "If it is omitted, internal character encoding is used"
There was a problem hiding this comment.
@WinterSilence Even with invalid encoding old implementation worked fine: https://3v4l.org/suLOs
There was a problem hiding this comment.
@rob006 you pass encoding, old implementation don't do it and use default encoding.
There was a problem hiding this comment.
@WinterSilence I'm passing invalid encoding just to prove you, that even with invalid encoding it is working fine...
There was a problem hiding this comment.
@rob006 not full implementation, just try cut $suffix = 'ЫВА-1234.doc'
There was a problem hiding this comment.
@rob006 is everything ok? (:
| $pos = mb_strrpos($path, '/'); | ||
| if ($pos !== false) { | ||
| return mb_substr($path, $pos + 1); | ||
| if (!empty($suffix)) { |
There was a problem hiding this comment.
!empty($suffix) is not the same as mb_strlen($suffix) > 0 - it behaves differently if you pass "0" as $suffix.
|
As we mentioned before this is again PR not following good practices so I'm closing it. Please extract the portion actually responsible for fixing the reported problem in separate PR. As for the so called "optimization" part - we already found some problems there and you apparently cannot admit its flaws so I suggest dropping it. But hey, you can always send another (proper) PR. |
|
@bizley "we" is you? As I have already said, personal grievances are irrelevant here. You can try current method with path not in 'UTF-8'(you can use something like 'Windows-1251') and pass 'ФЫВА-1234.doc'. |
|
@WinterSilence I agree with how hard is to review many things at once in a single PR. Noone questioned that there's an issue the PR is fixing. It's just doing too much at once additionally to it. Splitting PR into several PRs will make everyone's life easier a lot. |
BaseStringHelper::basename(): passing normalized path to PHPbasename()BaseStringHelper::dirname(): if path '/dir/sub-dir/', method return '/dir' instead '/dir/sub-dir'