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.

Sign up for GitHub

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

Jump to bottom

dev/core#2556 Rename extension org.civicrm.search -> org.civicrm.search_kit #20124

Merged
merged 2 commits into from
Apr 23, 2021
Merged

dev/core#2556 Rename extension org.civicrm.search -> org.civicrm.search_kit #20124

merged 2 commits into from
Apr 23, 2021

Conversation

colemanw
Copy link
Member

Overview

Rename extension to avoid a namespace conflict in Drupal 7 (and it's a more descriptive name anyway).
See https://lab.civicrm.org/dev/core/-/issues/2556

@civibot
Copy link

civibot bot commented Apr 22, 2021

(Standard links)

@civibot civibot bot added the master label Apr 22, 2021
colemanw
colemanw commented Apr 22, 2021
View reviewed changes
ext/search_kit/sql/auto_install.sql
@@ -64,6 +63,6 @@ CREATE TABLE `civicrm_search_display` (
)

, CONSTRAINT FK_civicrm_search_display_saved_search_id FOREIGN KEY (`saved_search_id`) REFERENCES `civicrm_saved_search`(`id`) ON DELETE CASCADE
) ROW_FORMAT=DYNAMIC ;
Copy link
Member Author

Choose a reason for hiding this comment

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

@seamuslee001 this is what happens when I rerun civix :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I've manually re-added the ROW_FORMAT=DYNAMIC bit.

@colemanw
dev/core#2556 Rename extension org.civicrm.search -> org.civicrm.sear…
fbf3a31 
…ch_kit

This avoids a namespace conflict in drupal 7, and is generally more descriptive
See https://lab.civicrm.org/dev/core/-/issues/2556
@demeritcowboy
Copy link
Contributor

I tested an upgrade and it worked but I got the error popup you get when you've deleted an installed extension's folder and I had to clear cache manually after the upgrade for it to work again. Maybe something about the order of when menu paths get rebuilt.

@colemanw
Copy link
Member Author

Thanks for testing @demeritcowboy. IMO it would certainly be nice to fix that issue, but not necessarily a blocker to merging this, as the extra cache flush isn't the worst thing in the world for the handful of sites using the extension (it's officially still beta).

@colemanw
Copy link
Member Author

@demeritcowboy I think I found a fix - pushed a second commit c486a70 which results in a better experience. In my testing, the error about a mising extension still pops up once after the upgrade, but then goes away by itself without a manual cache clear, and the search kit page works right away (previously it couldn't be found prior to flushing caches several times).

@demeritcowboy
Copy link
Contributor

Cool. Will take a look.

@colemanw
Copy link
Member Author

@totten I'm a little surprised the upgrader doesn't flush the extension cache per c486a70 as a standard part of the upgrade.

@demeritcowboy
Copy link
Contributor

Ok thanks retested and yes it's better. Didn't even get any popups.
I didn't test whether search kit still fully works, just a quickie contact test.

@seamuslee001
Copy link
Contributor

I'm going to merge based on Dave D's testing

@seamuslee001 seamuslee001 merged commit dedd7a3 into civicrm:master Apr 23, 2021
@seamuslee001 seamuslee001 deleted the renameSearchKit branch April 23, 2021 07:15
@colemanw
Copy link
Member Author

Thanks @demeritcowboy !

@colemanw colemanw mentioned this pull request Nov 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colemanw @demeritcowboy @seamuslee001