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

[5.1] Correct message type for Redirect plugin state #43402

Merged
merged 6 commits into from May 11, 2024

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Apr 30, 2024

Pull Request for Issue #43298 .

Summary of Changes

Use "notice" instead of "warning" for informative messages.

Testing Instructions

Please follow #43298

Actual result BEFORE applying this Pull Request

Yellow message

Expected result AFTER applying this Pull Request

Blue message

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org:
  • No documentation changes for manual.joomla.org needed

Reference:

@brianteeman
Copy link
Contributor

This does not solve #43298

This is how it should be - note that the message is completely wrong in test 3 and in test 2 it should be info not warning

@Fedik
Copy link
Member Author

Fedik commented Apr 30, 2024

it should be info not warning

It is "notice", as it was before in 5.0 and in j4.

@Fedik
Copy link
Member Author

Fedik commented Apr 30, 2024

tbh, this messages is useless, why do we have them in first place?
Only a warning about disabled plugin is good enough.

@brianteeman
Copy link
Contributor

Please READ the report more carefully. The messages are NOT useless.

I realise that there is a lot of variants but currently AND with this PR the following message is completely wrong!!!

When the plugin is enabled any redirects that have been created in the component will take effect.
When the plugin is enabled there is an additional option to collect urls. Collecting urls is independent of redirecting urls.

It says
The Redirect System Plugin is disabled. It needs to be enabled for this component to work.
It should say
The Redirect Plugin is enabled. The 'Collect URLs' option in the Redirect System Plugin is disabled. Error page URLs will not be collected by this component.

I am wondering now if you did not realise the complete use of the plugin and component when you rewrote this part of the code as it makes no sense.

The current code as changed in #42447 is simply wrong.

You never check if the plugin is enabled or disabled. You only check if the plugin exists

@Fedik
Copy link
Member Author

Fedik commented Apr 30, 2024

It is (almsot) 1:1 copy of what was before:
Old:

$pluginEnabled = PluginHelper::isEnabled('system', 'redirect');
$collectUrlsEnabled = RedirectHelper::collectUrlsEnabled();
// Show messages about the enabled plugin and if the plugin should collect URLs
if ($pluginEnabled && $collectUrlsEnabled) {
$this->app->enqueueMessage(Text::sprintf('COM_REDIRECT_COLLECT_URLS_ENABLED', Text::_('COM_REDIRECT_PLUGIN_ENABLED')), 'notice');
} else {
$redirectPluginId = RedirectHelper::getRedirectPluginId();
$link = HTMLHelper::_(
'link',
'#plugin' . $redirectPluginId . 'Modal',
Text::_('COM_REDIRECT_SYSTEM_PLUGIN'),
'class="alert-link" data-bs-toggle="modal" id="title-' . $redirectPluginId . '"'
);
if ($pluginEnabled && !$collectUrlsEnabled) {
$this->app->enqueueMessage(
Text::sprintf('COM_REDIRECT_COLLECT_MODAL_URLS_DISABLED', Text::_('COM_REDIRECT_PLUGIN_ENABLED'), $link),
'notice'
);
} else {
$this->app->enqueueMessage(Text::sprintf('COM_REDIRECT_PLUGIN_MODAL_DISABLED', $link), 'error');
}
}

New:

$collectUrlsEnabled = RedirectHelper::collectUrlsEnabled();
$redirectPluginId = $this->redirectPluginId;
// Show messages about the enabled plugin and if the plugin should collect URLs
if (!$redirectPluginId && $collectUrlsEnabled) {
$app->enqueueMessage(Text::sprintf('COM_REDIRECT_COLLECT_URLS_ENABLED', Text::_('COM_REDIRECT_PLUGIN_ENABLED')), 'warning');
} else {
$popupOptions = [
'popupType' => 'iframe',
'textHeader' => Text::_('COM_REDIRECT_EDIT_PLUGIN_SETTINGS'),
'src' => Route::_('index.php?option=com_plugins&client_id=0&task=plugin.edit&extension_id=' . $redirectPluginId . '&tmpl=component&layout=modal', false),
];
$link = HTMLHelper::_(
'link',
'#',
Text::_('COM_REDIRECT_SYSTEM_PLUGIN'),
[
'class' => 'alert-link',
'data-joomla-dialog' => $this->escape(json_encode($popupOptions, JSON_UNESCAPED_SLASHES)),
'data-checkin-url' => Route::_('index.php?option=com_plugins&task=plugins.checkin&format=json&cid[]=' . $redirectPluginId),
'data-close-on-message' => '',
'data-reload-on-close' => '',
],
);
if (!$redirectPluginId && !$collectUrlsEnabled) {
$app->enqueueMessage(
Text::sprintf('COM_REDIRECT_COLLECT_MODAL_URLS_DISABLED', Text::_('COM_REDIRECT_PLUGIN_ENABLED'), $link),
'warning'
);
} else {
$app->enqueueMessage(Text::sprintf('COM_REDIRECT_PLUGIN_MODAL_DISABLED', $link), 'error');
}
}

Maybe something wrong with redirectPluginId or with that if() condition, who knows.

@brianteeman
Copy link
Contributor

Its not the same - this is where the mistake is

ORIG

$pluginEnabled = PluginHelper::isEnabled('system', 'redirect');
$collectUrlsEnabled = RedirectHelper::collectUrlsEnabled();

NEW

$collectUrlsEnabled = RedirectHelper::collectUrlsEnabled();
$redirectPluginId = $this->redirectPluginId;

redirectPluginId gets the ID of the plugin
pluginEnabled gets the state of the plugin

@Fedik
Copy link
Member Author

Fedik commented Apr 30, 2024

redirectPluginId gets the ID of the plugin

It is there only when the plugin is disabled

if (!(PluginHelper::isEnabled('system', 'redirect') && RedirectHelper::collectUrlsEnabled())) {
$this->redirectPluginId = RedirectHelper::getRedirectPluginId();
}

@brianteeman
Copy link
Contributor

I give up - I am clearly unable to explain what should be very obvious. Your changed code in #42447 produces the wrong message and this PR makes no changes to correct that.

Instead of saying

The Redirect Plugin is enabled. The 'Collect URLs' option in the Redirect System Plugin is disabled. Error page URLs will not be collected by this component.

Your code says

The Redirect System Plugin is disabled. It needs to be enabled for this component to work.

@Fedik
Copy link
Member Author

Fedik commented Apr 30, 2024

I give up

Don't give up

@brianteeman
Copy link
Contributor

I dont know how else to explain the obvious

@Fedik
Copy link
Member Author

Fedik commented Apr 30, 2024

Should work now.
See, it was easy ;)

@brianteeman
Copy link
Contributor

image

@Fedik
Copy link
Member Author

Fedik commented Apr 30, 2024

But messages are good? :)
Changed that also, check again.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 3e9f660


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

1 similar comment
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 3e9f660


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

@alikon
Copy link
Contributor

alikon commented May 2, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 2, 2024
@LadySolveig LadySolveig added this to the Joomla! 5.1.1 milestone May 10, 2024
@bembelimen bembelimen merged commit 9976a6e into joomla:5.1-dev May 11, 2024
1 of 3 checks passed
@bembelimen
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 11, 2024
@Fedik Fedik deleted the fix-43298 branch May 11, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect component: Wrong error message shown when not collecting URLs
8 participants