Skip to content

Inherit Itemid from current URL if option matches#19498

Closed
OctavianC wants to merge 1 commit intojoomla:stagingfrom
OctavianC:patch-10
Closed

Inherit Itemid from current URL if option matches#19498
OctavianC wants to merge 1 commit intojoomla:stagingfrom
OctavianC:patch-10

Conversation

@OctavianC
Copy link
Contributor

@OctavianC OctavianC commented Jan 31, 2018

Pull Request for Issue #19497 .

Summary of Changes

Due to #19099 URLs no longer inherit Itemid. They SHOULD if the option matches, otherwise that PR introduced a B/C change and all URLs should be passed to JRoute::_() without specifying option to keep this compatible (eg. index.php?view=test instead of index.php?option=com_test&view=test)

Testing Instructions

See #19497

@Bakual
Copy link
Contributor

Bakual commented Jan 31, 2018

Imho, that PR #19099 was wrong to begin with and tried to fix an "issue" which actually is a misconfigured site. From my understanding it allows URLs to be generated without any Itemid, which is plain wrong.

So while this PR here tries to fix an issue, reverting the original PR likely still is better.

@OctavianC
Copy link
Contributor Author

+1 for reverting the original PR

@ghost
Copy link

ghost commented Jan 31, 2018

@goncatin please mark your Test as successfully:

  • open Issue Tracker
  • Login with your github-Account
  • Click on blue "Test this"-Button above Authors-Picture
  • mark your Test as successfully
  • hit "submit test result"

@goncatin
Copy link

I have tested this item ✅ successfully on e4a5bc6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19498.

@sandewt
Copy link
Contributor

sandewt commented Jan 31, 2018

I have tested this item ✅ successfully on e4a5bc6

BEFORE APPLY PATCH

string(92) "/bugtesten1/joomla-cms-staging/joomla-cms-staging/index.php?option=com_content&view=test"
string(107) "/bugtesten1/joomla-cms-staging/joomla-cms-staging/index.php?view=test&option=com_content&Itemid=481"

AFTER APPLY PATCH

string(107) "/bugtesten1/joomla-cms-staging/joomla-cms-staging/index.php?option=com_content&view=test&Itemid=481"
string(107) "/bugtesten1/joomla-cms-staging/joomla-cms-staging/index.php?view=test&option=com_content&Itemid=481"

PHP Version 7.1.9
Joomla! 3.8.4-rc2 Release Candidate


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19498.

@ghost
Copy link

ghost commented Jan 31, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 31, 2018
@brianteeman
Copy link
Contributor

I am going to remove the RTC for the moment as it is not yet clear is the entire original pr needs to be reverted and not just this part

@brianteeman brianteeman added PR-staging and removed RTC This Pull Request is Ready To Commit labels Jan 31, 2018
@bembelimen
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on e4a5bc6

The original PR should be reverted completly


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19498.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 31, 2018
@bembelimen
Copy link
Contributor

@OctavianC perhaps you could update your PR and add all changes to revert?

@OctavianC
Copy link
Contributor Author

I'd love to but it's not up to me at this point - see #19504

@ggppdk
Copy link
Contributor

ggppdk commented Jan 31, 2018

@bembelimen

can you provide more information on your unsuccessful test ?

@infograf768
Copy link
Member

rreset to pending depending on revert #19512


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19498.

@infograf768 infograf768 removed PR-staging RTC This Pull Request is Ready To Commit labels Feb 1, 2018
@ggppdk ggppdk mentioned this pull request Feb 2, 2018
@brianteeman
Copy link
Contributor

Closed as the routing changes have been reverted for 3.8.5 with #19512

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

Comments