Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display a warning whenever an menu item's alias has changed #13856

Closed
wants to merge 4 commits into from

Conversation

marrouchi
Copy link
Contributor

@marrouchi marrouchi commented Feb 2, 2017

Display a warning message whenever a menu item's alias has been updated. The message suggests to the admin user to create a new redirection to the old alias in order to avoid 404 errors from existing urls.

Pull Request for Issue #13851.

Summary of Changes

  1. Added two methods to the redirect plugin:
  • onContentbeforeSave : Check if the menu item's alias has changed. If it's the case, then display a warning message that suggests to the admin user to create a redirection to the old alias.
  • onContentPrepareForm : Alter menu item edit form whenever there's GET params (old_alias & new_alias) in order to set default values for the fields old_url & new_url
  1. An enable/disable field (Watch Alias) has been added to the plugin config form if there's need a disable this feature.

Testing Instructions

First, make sure that the redirect plugin is enabled.

When enabled :

  1. Edit an existing menu item and change the alias. Then hit the Save button.
  2. A warning message should be displayed that says : "You should create a new redirection to avoid 404 errors that could be generated by the old alias's urls : Create a new redirection"
  3. Click on the link Create a new redirection and you should be redirected to a new redirection form with the two fields old_url & new_url filled with default values. Hit the Save button.
  4. Access the old URL in frontend (that uses the old alias) and you should be redirected to the new one.

When disabled :

  1. Go the redirect plugin config form and disable Watch Alias
  2. Back to the menu manager, Edit an existing menu item and change the alias. Then hit the Save button.
  3. There will be no warning message.

Documentation Changes Required

No.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Feb 2, 2017
@tonypartridge
Copy link
Contributor

@marrouchi great minds think a like ;-) I just created an issue today about this

issue #13851

I was thinking of going one step further and auto-creating the redirect if a user wishes. I'll run some tests on this.

@marrouchi
Copy link
Contributor Author

@tonypartridge Yes i saw your issue and i've coded a PR right away :)

@marrouchi marrouchi changed the title [imp] New plugin that displays a warning whenever an menu item's has changed [imp] New plugin that displays a warning whenever an menu item's alias has changed Feb 2, 2017
@tonypartridge
Copy link
Contributor

tonypartridge commented Feb 2, 2017

@marrouchi I am just wondering if a plugin is needed. Can we not just adjust the com_menus component? Since the plugin is called on every save when it is only needed on a menu item save.

@marrouchi
Copy link
Contributor Author

I thought of this approach would be better to keep the menu item controller clean, since this feature is related to two components (com_menus & com_redirect).

@tonypartridge
Copy link
Contributor

I see your point completely, but it's then a plugin which is also then still requires both components. But both components ship with the core.

@zero-24 @mbabker what are your thoughts on this?

I was thinking of checking if com_redirects plugin is enabled and if so basically doing what Marrouchi has done. But within the component controller opposed to a plugin?

@mbabker
Copy link
Contributor

mbabker commented Feb 2, 2017

Why not throw it in the existing redirect plugin? The notice seems like a good idea to me and that plugin already has to be active to make use of the system, so...

@tonypartridge
Copy link
Contributor

Duhh good idea! @mbabker

@marrouchi do you want to do the changes?

@marrouchi
Copy link
Contributor Author

Yes good idead ! Should i create a new PR ?

@tonypartridge
Copy link
Contributor

I don't think so? Just remove this plugin from your GIT and add the changes to the system and push your changes they should come through for us to review again.

@ghost
Copy link

ghost commented Feb 2, 2017

One should only get a message or action link (whatever) if sef is enabled.
And there should be an option in the plugin to deactivate this feature completely, please. Because of other extensions that also deal with aliases.

@tonypartridge
Copy link
Contributor

tonypartridge commented Feb 2, 2017 via email

@marrouchi marrouchi closed this Feb 2, 2017
@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Feb 2, 2017
@marrouchi marrouchi reopened this Feb 2, 2017
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Feb 2, 2017
@marrouchi
Copy link
Contributor Author

Done ! I've added a enable/disable button of this features in the redirect plugin config form.

Ready to test 👍

@marrouchi marrouchi changed the title [imp] New plugin that displays a warning whenever an menu item's alias has changed [imp] Display a warning whenever an menu item's alias has changed Feb 2, 2017
@ghost
Copy link

ghost commented Feb 2, 2017

I have tested this item 🔴 unsuccessfully on 429b80d

Point 1-3 of Testing Instructions works.

Not working Point 4: Using old Url get redirected on Homepage. Using modern Router.

Another Point: If User have Redirect-Plugin disabled, they got no Hint.


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

@marrouchi
Copy link
Contributor Author

@franz-wohlkoenig This PR does not alter the redirection mechanism. Redirection does not seem to work when modern routing is enabled and that's an new issue that must be addressed. To confirm this, please revert the patch and try a redirection when having modern routing enabled.

@ghost
Copy link

ghost commented Feb 4, 2017

Test on legacy Router, Plugin "Redirect" enabled.

Got on Point 4 (using old alias using-joomla) a 404 Category not found. Category is enabled, in Plugin is a new, disabled Link index.php/using-joomla without new URL.

Test on:

Joomla! 3.7.0-beta1
macOS Sierra, 10.12.3
Firefox 50.1.0
PHP 7.0.4
MySQLi 5.5.53-0

@brianteeman
Copy link
Contributor

Can you look at resolving the conflicts so that this can be tested please.

@raoulyemetio
Copy link

raoulyemetio commented Jul 23, 2018

I have tested this item 🔴 unsuccessfully on b9ac171

I apply the patch, watch alias was enabled
then i changed an existing menu item
but i got no warning message
@icampus


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

@coolcat-creations
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on b9ac171

I would love to have this feature, but i had an error on testing.

0 Using $this when not in object context
/Applications/MAMP/htdocs/joomla-cms/libraries/src/Application/CMSApplication.php:369

@YEMETIO if you restest later open the plugin and save the settings


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

@fatmaakay
Copy link
Contributor

I have tested this item ✅ successfully on b9ac171


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

2 similar comments
@fatmaakay
Copy link
Contributor

I have tested this item ✅ successfully on b9ac171


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

@durubayram
Copy link

I have tested this item ✅ successfully on b9ac171


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

@ghost
Copy link

ghost commented Jul 23, 2018

Ready to Commit after two successful tests considering 2 unsuccessful tests so Maintainers have to decide (as always).

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 23, 2018
@durubayram
Copy link

I have tested this item 🔴 unsuccessfully on b9ac171


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

@brianteeman
Copy link
Contributor

please remove the rtc status

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 23, 2018
@Quy
Copy link
Contributor

Quy commented Jul 23, 2018

Removed RTC


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

@fatmaakay
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on b9ac171


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

@coolcat-creations
Copy link
Contributor

@marrouchi
Can you change the Line I reviewed, and fix the Code Style for 3.8 ?

  • Would be great to have this watcher also on trashing and on unpublishing the item.
    Thank you for this great addition.

* @return boolean true if function not enabled, is in frontend or is new. Else true or
* false depending on success of save function.
*
* @since 1.6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to __DEPLOY_VERSION__

*
* @return boolean
*
* @since 1.6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to __DEPLOY_VERSION__

{
if (!($form instanceof JForm))
{
$this->_subject->setError('JERROR_NOT_A_FORM');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing language string

{
if (!$isNew && $context == 'com_menus.item' && $this->params->get('watch_alias'))
{
$menu = & JSite::getMenu();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this line to

		$menu = (new \Joomla\CMS\Application\SiteApplication)->getMenu();

to make the PR work.

@ghost ghost added the J3 Issue label Apr 5, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@ghost ghost changed the title [imp] Display a warning whenever an menu item's alias has changed Display a warning whenever an menu item's alias has changed Apr 19, 2019
@HLeithner
Copy link
Member

Hi @marrouchi,

as this is a new feature it will not go into J3.x series because of the feature freeze. Also it seams it doesn't work (4 failing tests).
If you still interested to bring this feature into Joomla please rebase the PR on J4 branch and correct the reported errors.

Thx for your work and time you invested into Joomla! I'm closing this PR and hope you find time to create a new one for J4.

@HLeithner HLeithner closed this Jun 30, 2019
@ghost ghost added the J4 Rebase label Jul 1, 2019
@HLeithner HLeithner added this to Not rebased for J4 in [3.x-4.0] Closed feature PRs Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet