User tests: Successful: Unsuccessful:
Please refer to the endless chat in #4433 and #5569
This reverts #4433 and implements JPath::Clean() on the only comparison that was causing an issue that #4433 was coded to fix.
While I still believe that we should do this the RIGHT way, this band aid approach seems to be the way others would prefer this to be fixed.
Labels |
Added:
?
|
The correct approach would be to not compare file paths with urls. Simple. The correct approach would be to define paths with the correct directory separator for the file system running on and to have / for urls. The correct approach would be a complete revisit of the whole paths/urls defined within Joomla.
I dont have time for that and neither do you.
No one is trying to "get at me" - No one has the "right way", the correct approach would be to revisit and ensure that urls are URLS with / and PATHS are paths correct to the operating system running the code.
It is NOT right to simply:
"adopt the convention that all path-containing strings should use the Unix directory separator (/),"
As although PHP does a good job at resolving paths, its not perfect. The operating system should be allowed to give PHP its correct DIRECTORY_SEPARATOR, else there is simply no point for PHP to have DIRECTORY_SEPARATOR constant.
This is blowing up more than it needs to be. There is a PR to revert the change that caused you an issue, and you have further provided PR to catch another instance where the slash makes a difference on windows (Ive not tested that). Lets leave it at that as mbaber says...
OK, as you wish, but please note that nobody was comparing paths to URLs here... the issue wasn't caused by that...
Unfortunately(?) I dont have access to a windows server so I cant test it
@brianteeman Soon you will have :-)
Thanks for going the extra mile and setting up the server
@test Using the IIS server Phil paid for and setup I have confirmed the issue and have confirmed that this OR resolves the issue.
Testing on an apache server with this PR applied has no negative effect
Status | Pending | ⇒ | Ready to Commit |
Two good tests setting to RTC
Labels |
Added:
?
|
The original pr you did was better than this. I'm just gonna close this.
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-01-03 00:44:23 |
The original pr you did was better than this. I'm just gonna close this.
No worries :-) Your call :)
So what is happening with the other PR #5566
On 3 January 2015 at 00:45, Phil Taylor notifications@github.com wrote:
The original pr you did was better than this. I'm just gonna close this.
No worries :-) Your call :)
—
Reply to this email directly or view it on GitHub
#5580 (comment).
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
That's a separate issue and that needs to be tested and if succesful merged
I am lost
On 3 January 2015 at 02:31, George Wilson notifications@github.com wrote:
That's a separate issue and that needs to be tested and if succesful merged
—
Reply to this email directly or view it on GitHub
#5580 (comment).
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
This one was an issue relating to a issue in JUri giving incorrect URL's. That was initially fixed in #4433 but then people moaned so this was something alternate.
#5566 is related to a different (but related) issue where a bug with a similar cause causes the favicon not to show cause of an issue in JDocument.
So the bugs are separate but they've been discussed together because they're related to the fact we've hardcoded forward slashes which doesn't work in some places
Ah ok thanks. I think it is the first reply that has confused me as it states that it fixes the same issue that the other PR attempts to fix
Labels |
Removed:
?
|
Phil, I tried your PR and indeed it solves my issue #5559
For the same issue I also prepared #5566 which has a different approach (it correctly prepares for comparison path-containing strings)
Either of the two solves #5559, but I think both should be applied: yours as re-instate for some important named constants the values that have worked so far, and mine as it correct a logic issue (when comparing path-containing strings we must be sure to compare apples to apples and oranges to oranges).
I'm not getting if you are referring to me with this, and I don't know what you have in mind when you talk about "the RIGHT way": as you know my right way would be to adopt the convention that all path-containing strings should use the Unix directory separator (
/
), as I sustain in my "proof of concept" PR #5569@test success
Thanks!