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

[4.0] Removing com_search #20637

Merged
merged 21 commits into from Jun 20, 2019
Merged

[4.0] Removing com_search #20637

merged 21 commits into from Jun 20, 2019

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jun 1, 2018

With the changes to com_finder that I proposed, Smart Search should finally be ready for prime time. Having two complete search implementations in Joomla is pretty stupid, especially considering that Smart Search is far superior to com_search and thus this PR removes com_search from Joomla.

This PR should only be merged when all other changes to com_finder have been implemented.

@Hackwar Hackwar requested a review from brianteeman as a code owner June 1, 2018 07:46
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Jun 1, 2018
@brianteeman
Copy link
Contributor

  1. I agree with this in principal
  2. How do we deal with upgrades to J4 when existing sites are using com_search modules and menu items
  3. The search statistics page in the admin for com_finder was mistakenly in com_search - this would need to be resolved as its now been deleted

@infograf768
Copy link
Member

infograf768 commented Jun 1, 2018

Still waiting for you to modify the stemmer PR...
Also, tests should be done with languages who don't have stemmers and are likely never to get any to see if we should really get rid of com_search.

Note: In any case this PR is incomplete. Missing stuff in script.php and in en-GB.localise.php
plus the other issues described by @brianteeman above

@laoneo laoneo changed the title [4.0] Removing com_search [4.0][On Hold] Removing com_search Jun 1, 2018
@Hackwar
Copy link
Member Author

Hackwar commented Jun 1, 2018

@brianteeman

How do we deal with upgrades to J4 when existing sites are using com_search modules and menu items

I'd say that we should migrate those automatically to com_finder, or is there something that would speak against that?

The search statistics page in the admin for com_finder was mistakenly in com_search - this would need to be resolved as its now been deleted

I'm working on implementing a new search statistics already. That will be another PR in the near future.

@infograf768

Also, tests should be done with languages who don't have stemmers and are likely never to get any to see if we should really get rid of com_search.

Even if there is no stemmer for a language, the results are still several magnitudes better than with com_search. There is no situation where com_search would be equal or even better than com_finder.

Note: In any case this PR is incomplete. Missing stuff in script.php and in en-GB.localise.php

Yes, I missed the stuff in en-GB.localise.php. From what I understand the lines in script.php are script-generated and I wouldn't want to touch that right now.

@mbabker
Copy link
Contributor

mbabker commented Jun 1, 2018

How do we deal with upgrades to J4 when existing sites are using com_search modules and menu items

Same way the decoupling of weblinks was done. We can remove from repo, the search component/module/plugins should be unprotected so they can be uninstalled from UI, and we need to ensure last 3.x release of com_search will at least function on a 4.0 build. Add a postinstall indicating the component is abandoned/removed and to migrate search platforms.

@infograf768
Copy link
Member

@mbabker
not exactly same situation specially if the ini files as well as the functions in localise.php are removed.
We must not forget that 4.0 should get totally new lang packs.

@mbabker
Copy link
Contributor

mbabker commented Jun 1, 2018

You're not going to upgrade from 3.9/10 to 4.0 and all of a sudden the already installed 3.x language packs stop working. So the INI thing is really no concern unless we put in the script.php file "uninstall all the com_search things" code. Which we can't sanely do without breaking sites because for those sites where the component and module are in active use it can and probably will cause problems.

The localise code is a concern that would have to be thought through.

@mbabker
Copy link
Contributor

mbabker commented Jun 1, 2018

Actually, the localise code only becomes a problem if you remove the corresponding hooks in the Language class. So even if the search code is gone in the en-GB class, if the hooks and default behaviors still exist in Language we'd be OK (at most it alters the search results for the component but it prevents a situation where upgrading the site in full removes something that is user configured somewhere; we don't exactly have a good track record with complex upgrade paths (see 1.5 -> 1.6) so we REALLY need to be careful if someone is going to push for a "well why don't we just delete the component from the database, remove menu items, etc." solution).

@infograf768
Copy link
Member

So, the part of localise.php which "could" possibly create B/C issues would only be (Here for Chinese Simplified)

	public static function getIgnoredSearchWords()
	{
		return array('啊', '呢', '嘛');
	}

@Hackwar
Copy link
Member Author

Hackwar commented Jun 3, 2018

I don't think that we have to do anything special here. 4.0 is a break in B/C and updating to 4.0 includes to switch the search solution to Smart Search.

@Hackwar
Copy link
Member Author

Hackwar commented Jun 6, 2018

The statistics for com_finder are now in this PR: #20681

…4search_remove

# Conflicts:
#	components/com_search/View/Search/HtmlView.php
#	installation/sql/mysql/joomla.sql
#	installation/sql/postgresql/joomla.sql
#	modules/mod_search/tmpl/default.php
@Hackwar
Copy link
Member Author

Hackwar commented Jun 20, 2018

Can we remove the "HOLD" on this PR?

@joomla-cms-bot joomla-cms-bot changed the title [4.0][On Hold] Removing com_search [4.0] Removing com_search Jun 20, 2018
@laoneo
Copy link
Member

laoneo commented Jun 21, 2018

4.0 is a break in B/C and updating to 4.0 includes to switch the search solution to Smart Search.

I do not agree here that the user which updates Joomla must make the switch manually. Joomla has to provide an upgrade path for this pr.

@infograf768
Copy link
Member

I suggest to wait anyway before taking off com_search as there are some aspects of finder that need special attention, I am thinking specially of the PR that would introduce the length choice for the tuple.
#20384 (comment)

@brianteeman
Copy link
Contributor

@laoneo as you have established a policy of merging things without them being complete or in some cases even working there is no reason at all for this not to be merged right now and the migration issue addressed in another pr

@laoneo
Copy link
Member

laoneo commented Jun 21, 2018

This PR should only be merged when all other changes to com_finder have been implemented.

@brianteeman Quote from the description of this pr. Not sure how your comment should help bringing this pr further. So please keep the focus and when you want to say something to me, then do it by PM and not in a public tracker. Thanks for understanding.

@brianteeman
Copy link
Contributor

@laoneo public comments on a public tracker should have public replies. you cant have your cake and eat it.

@Hackwar
Copy link
Member Author

Hackwar commented Jun 21, 2018

@laoneo The problem with an automatic upgrade path is, that it is nearly impossible. The user will have to do changes himself, for example checking if every component has a finder plugin that he wants, building the initial index and adapting template overrides from com_search onto com_finder. It wont be enough to simply change the type of module from mod_search to mod_finder and likewise with menu items of com_search. By doing parts of this for the user on upgrade, I fear that we will have lots and lots and lots of users who don't even notice the switch over and thus will not have a (complete) index or all necessary finder plugins.

However, while reviewing the PR just now, we do need indeed some changes. We need a PR that deprecates com_search in 3.9 and also adds a notice that you should prepare to switch over from com_search to com_finder and we need to properly uninstall the search stuff in 4.0.

@infograf768 I do would say that thr finder changes have progressed far enough that we can drop com_search now. The changes that have not been merged yet are very near RTC and the rest of the PRs that I have planned are just small improvements, not major changes like we had in the last few weeks.

@laoneo
Copy link
Member

laoneo commented Jun 21, 2018

@Hackwar then we need also do an announcement as I would say this is a BC break which effects the end user and not extensions. Do you suggest to even recommend to upgrade on J3 first before doing the upgrade to J4, is then a reindex needed after the upgrade?

@laoneo
Copy link
Member

laoneo commented Aug 4, 2018

Decouple it sound like a good plan.

Copy link
Contributor

@brianteeman brianteeman left a comment

Choose a reason for hiding this comment

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

languuage string changes are ok

@ghost ghost added the J4 Issue label Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
…4search_remove

# Conflicts:
#	administrator/components/com_search/Controller/DisplayController.php
#	administrator/components/com_search/Controller/SearchesController.php
#	administrator/components/com_search/Helper/SearchHelper.php
#	administrator/components/com_search/Model/SearchesModel.php
#	administrator/components/com_search/View/Searches/HtmlView.php
#	administrator/components/com_search/forms/filter_searches.xml
#	administrator/components/com_search/helpers/search.php
#	administrator/components/com_search/search.xml
#	administrator/components/com_search/tmpl/searches/default.php
#	administrator/language/en-GB/en-GB.com_search.ini
#	administrator/language/en-GB/en-GB.com_search.sys.ini
#	administrator/language/en-GB/en-GB.plg_search_categories.ini
#	administrator/language/en-GB/en-GB.plg_search_categories.sys.ini
#	administrator/language/en-GB/en-GB.plg_search_contacts.ini
#	administrator/language/en-GB/en-GB.plg_search_contacts.sys.ini
#	administrator/language/en-GB/en-GB.plg_search_content.ini
#	administrator/language/en-GB/en-GB.plg_search_content.sys.ini
#	administrator/language/en-GB/en-GB.plg_search_newsfeeds.ini
#	administrator/language/en-GB/en-GB.plg_search_newsfeeds.sys.ini
#	administrator/language/en-GB/en-GB.plg_search_tags.ini
#	administrator/language/en-GB/en-GB.plg_search_tags.sys.ini
#	components/com_search/Controller/DisplayController.php
#	components/com_search/Model/SearchModel.php
#	components/com_search/View/Search/HtmlView.php
#	components/com_search/View/Search/OpensearchView.php
#	components/com_search/router.php
#	components/com_search/tmpl/search/default.php
#	components/com_search/tmpl/search/default_error.php
#	components/com_search/tmpl/search/default_form.php
#	components/com_search/tmpl/search/default_results.php
#	installation/sql/mysql/joomla.sql
#	installation/sql/postgresql/joomla.sql
#	language/en-GB/en-GB.com_search.ini
#	language/en-GB/en-GB.mod_search.ini
#	language/en-GB/en-GB.mod_search.sys.ini
#	libraries/src/Helper/SearchHelper.php
#	modules/mod_search/Helper/SearchHelper.php
#	modules/mod_search/mod_search.php
#	modules/mod_search/mod_search.xml
#	modules/mod_search/tmpl/default.php
#	plugins/search/categories/categories.php
#	plugins/search/categories/categories.xml
#	plugins/search/contacts/contacts.php
#	plugins/search/contacts/contacts.xml
#	plugins/search/content/content.php
#	plugins/search/content/content.xml
#	plugins/search/newsfeeds/newsfeeds.php
#	plugins/search/newsfeeds/newsfeeds.xml
#	plugins/search/tags/tags.php
#	plugins/search/tags/tags.xml
@Quy
Copy link
Contributor

Quy commented Jun 12, 2019

In classmap.php, remove the following?

JLoader::registerAlias('JSearchHelper', '\\Joomla\\CMS\\Helper\\SearchHelper', '5.0');

@Hackwar
Copy link
Member Author

Hackwar commented Jun 12, 2019

done.

@Quy
Copy link
Contributor

Quy commented Jun 15, 2019

Delete \administrator\components\com_search\services\provider.php?

…4search_remove

# Conflicts:
#	administrator/components/com_search/tmpl/searches/default.php
#	components/com_search/View/Search/HtmlView.php
#	plugins/search/categories/categories.php
#	plugins/search/content/content.php
@Quy
Copy link
Contributor

Quy commented Jun 17, 2019

@brianteeman
Copy link
Contributor

@Quy in an ideal world yes but this hasnt been done in j4 during this alpha stage. It can be done in one go later

@wilsonge wilsonge merged commit 6dd88af into joomla:4.0-dev Jun 20, 2019
@wilsonge
Copy link
Contributor

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jun 20, 2019
@Hackwar Hackwar deleted the j4search_remove branch June 20, 2019 16:24
@laoneo
Copy link
Member

laoneo commented Jun 21, 2019

Whuah, big step done!

@brianteeman
Copy link
Contributor

Thanks

@Hackwar
Copy link
Member Author

Hackwar commented Jun 21, 2019

Thank you!

@alikon
Copy link
Contributor

alikon commented Jun 21, 2019

didn't we have now the nested tables (#__asset, #__menu) in a not good state ?

@mbabker
Copy link
Contributor

mbabker commented Jun 28, 2019

Merges like this one are exactly why there should be something similar to https://symfony.com/blog/category/living-on-the-edge on the joomla.org network. Because this major merge isn't going to be documented anywhere except as a footnote of an alpha release (if even that much). And anyone who has listened to me knows I've been saying for years we need that type of proactive blog content and that work on upcoming releases needs to stop being buried in this repository until the stable release goes out when suddenly it's OK to put all the marketing effort behind things (which is way too late).

@Hackwar
Copy link
Member Author

Hackwar commented Jun 28, 2019

I agree that we should inform people more and I'm happy to write blogposts for the features that I'm contributing.

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
None yet
Development

Successfully merging this pull request may close these issues.

None yet