Skip to content

Add a redirection for the menu item alias#22432

Merged
mbabker merged 1 commit intojoomla:stagingfrom
csthomas:redirect_on_menu_alias
Oct 6, 2018
Merged

Add a redirection for the menu item alias#22432
mbabker merged 1 commit intojoomla:stagingfrom
csthomas:redirect_on_menu_alias

Conversation

@csthomas
Copy link
Contributor

Pull Request for Issue #19340 and #19624

Summary of Changes

Proposal to solve problems.

Testing Instructions

See issues.

Expected result

When you go to the link that is an alias then you will be redirected to the correct page.

Actual result

You remain on a page of the menu item alias.

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 8c5c907

It works fine here.


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

@ralain
Copy link
Contributor

ralain commented Oct 4, 2018

I have tested this item ✅ successfully on 8c5c907

Works as expected. Thanks!


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

@infograf768
Copy link
Member

@mbabker
Please check before merging that there are no unwanted consequences.

@Quy
Copy link
Contributor

Quy commented Oct 4, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added RTC This Pull Request is Ready To Commit PR-staging and removed RTC This Pull Request is Ready To Commit labels Oct 4, 2018
@csthomas csthomas deleted the redirect_on_menu_alias branch October 7, 2018 19:21
@sanderpotjer
Copy link
Member

This change has unwanted consequences for us... An alias is not always necessarily a redirect. We are using aliases on purpose and don't want to have a 301 redirect for it (otherwise we would have created a redirect).

An example: we generate AMP-equivalents of pages by using the Alias menu-item. Thanks to the "Template Style" setting of the alias menu-item we can load a different (amp-focussed) template for that page. So e.g.:

  • website.com/article: menu-type article
  • website.com/amp/article: menu-type alias, with a different template selected and alias to "article".

Joomla 3.8.13:
website.com/amp/article is showing the article with the different (AMP) template

Joomla 3.9.0:
website.com/amp/article is redirected to website.com/article

If the "alias" should be considered as "redirect" after this patch, I don't see why we are still offering options for selecting the template style.

I do think we have two options here:
1: Revert this PR, and introduce a new System menu-type "Redirect" besides "Alias".
2: Add a new toggle option "Redirect": Yes/No, so you can select if the menu-item needs to be redirected or not.

@csthomas
Copy link
Contributor Author

csthomas commented Nov 5, 2018

I like point 2 more, but I'm not sure if the field label "Redirect" will be understandable.

@PatrickExact
Copy link

This change affects our site as well. We are using a similar set-up as @sanderpotjer describes.

Option 2 should fix this change for sure.

@csthomas
Copy link
Contributor Author

I will try to prepare a new PR with option 2

@phproberto
Copy link
Contributor

This change broke our site.

Extensions using jQuery.ajax method like:

				jQuery.ajax({
					context:this,
					url: 'index.php?option=com_mycomponent&task=mycontroller.method&format=raw',
					dataType: 'json',
					data: {start: start.format(),end: end.format()},
					success: function(doc) {
						// Do something
					}
				});

Are now causing a redirection. It also happens with POST requests. I know using urls in javascript without using the routing system are not technically ok but they are actually used by extensions and this breaks things.

I had to "hack" core to revert this.

@csthomas
Copy link
Contributor Author

Proposed solution at #23278

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