- Issue created by @marcus_johansson
- First commit to issue fork.
- 🇬🇧United Kingdom scott_euser
This works, but:
- Needs further testing as we need to make sure we get this right
- Needs update hooks in the external modules just like the one added to Pinecone: https://www.drupal.org/project/ai_vdb_provider_pinecone/issues/3483565 📌 Update hook from vdb_provider_pinecone setting to ai_vdb_provider_pinecone Active
- Needs to have further deletions of all moved modules
- Needs to have further deletions from composer.json
- Needs to have further deletions in docs folder
So sort of needs work/needs review dual state.
The files to look at to review:
- ai.install -> in particular see the Scenarios bit
- ai.module -> prevents accidental uninstall; see the comments, the user could really want to uninstall and not switch, so there is a route
I put detailed comments in both of those there.
The deletions add noise to the MR, but are needed for tests to pass since composer dependency gets removed (in example of pinecone so far).
Temporary alternative:
One alternative has been suggested of setting all submodule new external module versions as composer dependencies. That kicks the can down the road as it doesn't force the switchover, so still update hook is needed so AI core module can enforce stopping usage of the old modules and let us safely delete them from here.It also means that sites then get a lot more top-level modules temporarily. Same amount of code as is of course, but some users may be unhappy with that.
So I think we should aim to try to get it working well with a full removal.
What I tried
I tried a couple runs at this with pinecone to test:- Enabled external pinecone module -> old one successfully uninstalled and config copied over: this is the simplest case
- Ran updb without any knowledge of new external module -> ai core told me to composer require the external pinecone module.
- Run updb with composer require of new external module -> ai core module triggered the install file of pinecone which then did (1)
Side note: I checked with Marcus in slack first before working on branch, to check he hadn't started since its assigned to him.
- 🇬🇧United Kingdom scott_euser
Not 100% sure if we need to leave placeholder info yml files in place for a bit...
e.g.lifecycle: deprecated lifecycle_link: "https://www.drupal.org/project/ai/issues/3483356"
- 🇬🇧United Kingdom scott_euser
Yep, its not possible to run e.g. `drush cr` without it because of the module name change. I think this must be why Drupal Core forces you update to the latest within Major version. I added back in the info.yml file. There is still a risk regardless of scenario that a user is on e.g. alpha8 or earlier, and doesn't update e.g. for a few modules to some point when we have removed the deprecated submodules though. In that case, we could either decide:
- Its alpha, its okay - the edge case will come to issue queue and find similar issue
- We increase semver to 2x, so user's will have to more consciously do it
(2) is probably the proper way and matches Core movement of modules from Core to contrib I think, but not a hill I'll die over...
I additionally added this morning hook_requirements as well, which happens even earlier than updb. Still the user can skip this, so I left that particular scenario in the updb hook.
-
marcus_johansson →
committed cd84887c on 1.0.x authored by
scott_euser →
Resolve #3483356 "Move to contrib"
-
marcus_johansson →
committed cd84887c on 1.0.x authored by
scott_euser →
Automatically closed - issue fixed for 2 weeks with no activity.