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][com_finder] check if finder content plugin is enabled #42299

Closed
wants to merge 18 commits into from

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Nov 6, 2023

Pull Request for Issue #42296 .

Summary of Changes

correct the test if finder content plugin is enabled

Testing Instructions

Disable finder plugins, then start indexer

Actual result BEFORE applying this Pull Request

no message about disbled plugin

Expected result AFTER applying this Pull Request

message about disbled plugin

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

@Fedik
Copy link
Member

Fedik commented Nov 6, 2023

I am sorry to say, but the fix is wrong.
Checking for plugins/content/finder is needed but you removing it here.

What probably needed to fix #42296 is to do an addittional check, whether at least one plugin is enabled in the finder group.

if (!PluginHelper::getPlugin('finder')) {
 $this->app->enqueueMessage('No Finder plugin found, please enable at least one', 'warning');
}

And this will be a new feature, not a bug fix.

@alikon
Copy link
Contributor Author

alikon commented Nov 6, 2023

no problem, you are right
...i've just forgot about the existence of plugin 'content/finder' too... 😨

I'll give it a closer look

@alikon alikon marked this pull request as draft November 6, 2023 18:30
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Nov 11, 2023
@alikon alikon changed the base branch from 4.4-dev to 5.1-dev November 11, 2023 07:34
@alikon alikon changed the base branch from 5.1-dev to 4.4-dev November 11, 2023 07:35
@alikon alikon marked this pull request as ready for review November 11, 2023 12:45
@HLeithner
Copy link
Member

I don't think that this should go in 4.4 because it's not a bug.

@alikon alikon changed the base branch from 4.4-dev to 5.0-dev November 23, 2023 09:30
@alikon alikon requested a review from laoneo as a code owner November 23, 2023 09:30
@alikon alikon changed the base branch from 5.0-dev to 5.1-dev November 23, 2023 09:31
@alikon alikon changed the title [4][com_finder] check if finder content plugin is enabled [5.1][com_finder] check if finder content plugin is enabled Nov 23, 2023
@alikon alikon removed the PR-4.4-dev label Nov 23, 2023
@alikon
Copy link
Contributor Author

alikon commented Nov 23, 2023

switched to 5.1

@fgsw
Copy link

fgsw commented Dec 8, 2023

I have tested this item ✅ successfully on a455afe


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

@alikon alikon marked this pull request as draft January 13, 2024 10:32
@alikon alikon marked this pull request as ready for review January 14, 2024 10:27
@alikon
Copy link
Contributor Author

alikon commented Jan 14, 2024

should be aligned now

@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Feb 20, 2024
@ufuk-avcu
Copy link

I have tested this item ✅ successfully on 273a98d


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

1 similar comment
@nielsnuebel
Copy link
Contributor

I have tested this item ✅ successfully on 273a98d


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

@richard67 richard67 removed the PBF Pizza, Bugs and Fun label Feb 24, 2024
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 24, 2024
@richard67 richard67 added the PBF Pizza, Bugs and Fun label Feb 24, 2024
@crimle
Copy link

crimle commented Feb 24, 2024

I have tested this item 🔴 unsuccessfully on 273a98d

Plugin Extension - Finder disabled
Components > Smart Search > Index > Button [Index]
There is no error message


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

@richard67
Copy link
Member

I have tested this item 🔴 unsuccessfully on 273a98dPlugin Extension - Finder disabled Components > Smart Search > Index > Button [Index] There is no error message

@crimle It needs to disable all plugins in the "Finder" group, i.e. all with names like "Finder - ...".

@dorisdreher
Copy link

I have tested this item 🔴 unsuccessfully on 273a98d


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

@richard67
Copy link
Member

I have tested this item 🔴 unsuccessfully on 273a98d

@dorisdreher Could you report back what has not worked? Maybe you've disabled the wrong plugins? Check also my previous comment.

@crimle
Copy link

crimle commented Feb 24, 2024

I have tested this item 🔴 unsuccessfully on 273a98dPlugin Extension - Finder disabled Components > Smart Search > Index > Button [Index] There is no error message

@crimle It needs to disable all plugins in the "Finder" group, i.e. all with names like "Finder - ...".
There is one plugin only when searching for «finder»

@dorisdreher
Copy link

I have tested this item ✅ successfully on 273a98d


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

1 similar comment
@crimle
Copy link

crimle commented Feb 24, 2024

I have tested this item ✅ successfully on 273a98d


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

@@ -148,6 +149,7 @@ COM_FINDER_INDEX_NO_DATA="No content has been indexed."
COM_FINDER_INDEX_OPTIMISE_FINISHED="Optimisation finished."
COM_FINDER_INDEX_PLUGIN_CONTENT_NOT_ENABLED="The Smart Search Content Plugin is disabled. Changes to content will not update the Smart Search index until the Plugin is enabled."
COM_FINDER_INDEX_PLUGIN_CONTENT_NOT_ENABLED_LINK="The %s is disabled. Changes to content will not update the Smart Search index if you do not enable this plugin."
COM_FINDER_INDEX_PLUGIN_FINDER_NOT_ENABLED_LINK="The %s are disabled. Changes to content will not update the Smart Search index if you do not enable these plugins."
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we make one (new?) constant to cover both cases (as they have the same text) and deprecate the other two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we can
but out of scope of this pr

@@ -154,6 +163,11 @@ public function display($tpl = null)
$this->finderPluginId = FinderHelper::getFinderPluginId();
}

// Check that the finder plugins are enabled
if (!PluginHelper::getPlugin('finder')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a personal opinion: I don't like this "loose" check here, but I'm aware that the getPlugin method sucks in general (not fault of this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can agree with you,
but if my search results are not wrong
in core,
we already have more than 100 of this ...

out of scope of this pr

@Quy Quy added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed RTC This Pull Request is Ready To Commit Updates Requested Indicates that this pull request needs an update from the author and should not be tested. PBF Pizza, Bugs and Fun labels Mar 2, 2024
@alikon alikon closed this Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators PR-5.1-dev Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet