[#26621] - Be more careful with str_replace
- Fixed in Code Base
- 29 Oct 2019
- Medium
- Build: staging
- # 26621
- Diff
- okonomiyaki3000:template-replacement-fix
- Pending Hound Hound is busy sniffing around... Details
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
Changed this:
$this->_path['template'] = str_replace($template, $layoutTemplate, $this->_path['template']);To this:
$this->_path['template'] = str_replace(JPATH_THEMES . '/' . $template, JPATH_THEMES . '/' . $layoutTemplate, $this->_path['template']);Because, let's say the path on to your site on the server is /beez3/something/something/mycoolsite/ for some reason. And let's say you're using the beez3 template (even though that is totally unrelated to the fact that 'beez3' appears in your path).
Then you have a particular menu item which needs to use a layout file from protostar. Now, in this case, you're going to have:
$template = 'beez3';
$layoutTemplate = 'protostar';So what do you think will happen to your path when you call str_replace on it? Bad things, man. Bad things. So instead of doing such simple string replace, I'm replacing the entire path up to and including $template with the same path but with $layoutTemplate instead.
Testing Instructions
Oh boy.... First of all, set your site up somewhere so that you have the exact name of your default template somewhere in the path to the site.
Next, have at least two templates installed and create a menu item that uses a layout override file from the other (not your default) template.
Try loading that page.
Expected result
Well it should just load a page without errors, right?
Actual result
Of course it will not find the layout file which should be at:
/beez3/something/something/mycoolsite/templates/protostar/html/...
Because it will be looking in:
/protostar/something/something/mycoolsite/templates/protostar/html/...
So, this PR fixes that.
Documentation Changes Required
Nope
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
I have tested this item
Code review. No need for real test, the issue is obvious for anyone who ever worked with replacing substrings in a safe way.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26621.
I have not tested this item.
Just saw Sharky's comment.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26621.
Thanks SharkyKZ, you basically just prevented every windows instance of Joomla! from breaking.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26621.
I have tested this item
works as advertised
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26621.
No, you don't have to use DIRECTORY_SEPARATOR because PHP automatically changes that. The code is fine. That whole DS was utter bullshit.
So since we have 2 tests, this can be set to RTC.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26621.
@SharkyKZ Then we have a serious problem, because we removed all DIRECTORY_SEPARATOR or DS from our code and all filepaths use / as separator. Just take any filepath and mix forward and backwards slashes and PHP will find that file. Works since PHP 4. But even regardless of that, we are adding the same prefix to both strings, so we might as well write "Gobbledegoop, this ain't no path! Go away!" instead of JPATH_THEMES . "/". It would end with the same result.
@Hackwar almost everything moved from DIRECTORY_SEPARATOR ;) https://github.com/joomla/joomla-cms/blob/staging/includes/defines.php
@SharkyKZ is correct in this specific case
Lol...The lines immediately preceding this if statement are:
$lang->load('tpl_' . $template, JPATH_BASE, null, false, true)
|| $lang->load('tpl_' . $template, JPATH_THEMES . "/$template", null, false, true);If '/' doesn't work on Windows, how are we ever loading that language file?
Nothing to do with finding files. It's purely about the use of str_replace().
fair enough, the replace wouldn't work because windows would have '\' in the path (JPATH_THEMES . '\') but it wouldn't break though would it? it would just not fix the issue this pr is trying to fix
in any case; @okonomiyaki3000, if you could update the pr we can test it again.
@SharkyKZ OK, now you've really lost me. Are you suggesting that str_replace() works differently on windows than other systems? I thought we're talking about directory separators. In the context of str_replace(), / and \ are just strings, they have no meaning. Directory separators only matter when using a string as a path to do something with the filesystem and, as people have pointed out here, you can just use / for that everywhere.
@euismod2336 Ah, OK... I kind of see what you're saying here... Let me check.
OK, finally I get it. Because $this->_path is full of paths that have been cleaned... We can only find (and should only replace with) clean paths. Got it.
| Labels |
Added:
?
|
||
I have tested this item
Now with DS for some windows love ;)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26621.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26621.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-10-29 14:25:42 |
| Closed_By | ⇒ | HLeithner | |
| Labels |
Added:
?
|
||
Thanks
Need to use
JPath::clean()or replace/withDIRECTORY_SEPARATORso this doesn't break on Windows.