? Success

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
1 Jan 2015

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.

avatar PhilETaylor PhilETaylor - open - 1 Jan 2015
avatar jissues-bot jissues-bot - change - 1 Jan 2015
Labels Added: ?
avatar smz
smz - comment - 1 Jan 2015

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).

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.

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!

avatar smz smz - test_item - 1 Jan 2015 - Tested successfully
avatar PhilETaylor
PhilETaylor - comment - 1 Jan 2015

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...

avatar smz
smz - comment - 1 Jan 2015

OK, as you wish, but please note that nobody was comparing paths to URLs here... the issue wasn't caused by that...

avatar PhilETaylor
PhilETaylor - comment - 2 Jan 2015

Despite the distractions - this PR should still be considered as a revert of the #4433 that seems to have broken other peoples crappy windows hosted sites, and yet still fix the original issue that #4433 was designed to fix. I dont see any issues here.

avatar brianteeman
brianteeman - comment - 2 Jan 2015

Unfortunately(?) I dont have access to a windows server so I cant test it


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5580.
avatar PhilETaylor
PhilETaylor - comment - 2 Jan 2015

@brianteeman Soon you will have :-)
screenshot 2015-01-02 20 17 02

avatar brianteeman
brianteeman - comment - 2 Jan 2015

Thanks for going the extra mile and setting up the server


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5580.
avatar brianteeman
brianteeman - comment - 2 Jan 2015

@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


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5580.
avatar brianteeman brianteeman - test_item - 2 Jan 2015 - Tested successfully
avatar brianteeman brianteeman - change - 2 Jan 2015
Status Pending Ready to Commit
avatar brianteeman
brianteeman - comment - 2 Jan 2015

Two good tests setting to RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5580.
avatar brianteeman brianteeman - change - 2 Jan 2015
Labels Added: ?
avatar wilsonge
wilsonge - comment - 3 Jan 2015

The original pr you did was better than this. I'm just gonna close this.

avatar wilsonge wilsonge - change - 3 Jan 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-01-03 00:44:23
avatar wilsonge wilsonge - close - 3 Jan 2015
avatar zero-24 zero-24 - close - 3 Jan 2015
avatar PhilETaylor
PhilETaylor - comment - 3 Jan 2015

The original pr you did was better than this. I'm just gonna close this.

No worries :-) Your call :)

avatar brianteeman
brianteeman - comment - 3 Jan 2015

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/

avatar wilsonge
wilsonge - comment - 3 Jan 2015

That's a separate issue and that needs to be tested and if succesful merged

avatar brianteeman
brianteeman - comment - 3 Jan 2015

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/

avatar wilsonge
wilsonge - comment - 3 Jan 2015

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

avatar brianteeman
brianteeman - comment - 3 Jan 2015

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

avatar zero-24 zero-24 - change - 1 Apr 2015
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment