- Issue created by @raffaelj
- π«π·France fmb PerpinyΓ , Catalonia, EU
Thanks for reporting this. Is it also happening with the latest Git version?
> Is it also happening with the latest Git version?
I don't know, but I guess so. If I remember correctly, the latest version requires Drupal 11. I can't install Drupal 11 (in my local docker test setup), because it requires SQLite >=3.45. Now I'm waiting for π database driver crashes during install (dependency not autoloaded) Needs review to get merged. Than I'll have a working throw-away dev setup for D11 again.
During my quick test with this module, it looked like it is not capable of translating admin/settings pages of self written modules locally. So I also lost interest in digging deeper.
When this module is compatible with D10 again or when I have a working D11 setup, I might have another look.
- π©πͺGermany donquixote
Hello.
I was able to reproduce with latest version of this module both with Drupal 10 and Drupal 11.For now the way to resolve it is to manually remove the field before uninstall.
Fingers crossed :)We need to think about what would be the optimal solution to implement in this module.
One thing to consider is that removing the field will cause data loss, because all the tokens per user are lost.
This is why there is some merit in letting the user manually delete the field.On the other hand, this deletion also would need to happen in a deployment. So it would need a custom update hook in the project to delete the field, which would be a burden on the site builder.
- π«π·France fmb PerpinyΓ , Catalonia, EU
Thanks for spending time on analyzing this bug. We do not really care about data deletion in this case. I would say it is expected to remove the key when the module is uninstalled. Generating a new token would not be that hard anyway.
- Merge request !14Draft: Issue #3472140: Add uninstall validator for l10n_client_contributor module. β (Closed) created by donquixote
- π©πͺGermany donquixote
donquixote β changed the visibility of the branch 3472140-uninstall-validator to hidden.
- π©πͺGermany donquixote
I had a look at the core comment module, which also introduces a field type 'comment', that can be added (manually) to other entities like nodes.
The field does not store the comments but rather the comment settings for that entity.Once a comment field has been created, the comment module can no longer be uninstalled.
The fields must be manually deleted first.The comment module has a `comment_uninstall()` function, which deletes all fields of the type 'comment'.
But it is unclear to me why this exists.
At the time we get to that hook, all the fields will be already manually removed.The main difference between comment and l10n_client_contributor is that comment fields are created manually, but the field from l10n_client_contributor is created automatically on install. This creates an asymmetry between install and uninstall.
We could add our own l10n_client_contributor_uninstall(). But that won't really do anything.
At the same time, I notice that since commit d941cd35 from π Replace xmlrpc for new localize.drupal.org Needs work , we are not using that field anymore.
So I would propose the following steps to "fix" this:
- (in this issue) Add an entry in the README.txt about uninstalling the module.
- (in a follow-up) Remove the field, or rethink it, based on how we interact with the l10n_server in the future.
- π©πͺGermany donquixote
What does this mean for an upgrade path, when a site has this module installed and wants to uninstall it?
I just tried it, it seems really simple for a site with config/sync:
- In your local instance, delete the field (manually), uninstall the module, and export (drush cex).
- Deploy and import config.No custom update hook was needed.
- π©πͺGermany donquixote
I tested again, this time I had data in the field.
Again, "drush config:import" successfully removed the field and uninstalled the module. - π©πͺGermany donquixote
(in this issue) Add an entry in the README.txt about uninstalling the module.
I notice that the submodule does not have a README.md or .txt.
Should we add one, or put everything in the main module README?
Or do we have a better place for this kind of documentation?We could also create an uninstall validator that adds additional information, but it seems overkill.
- π«π·France fmb PerpinyΓ , Catalonia, EU
At the same time, I notice that since commit d941cd35 from #3395327: Replace xmlrpc for new localize.drupal.org, we are not using that field anymore.
Mmmh, but we will need a way to store credentials for a given user in π Localize client authentication with d.o infrastructure Active . But then, of course, nothing forces us to use a field if we judge this is not an appropriate solution (personally, I do not think it is, though it certainly appears to be the easiest method at first).
If for whatever reason we decide to stick with a field, we would need a way to advise users to manually delete the field first, just like we need to manually delete content when removing an entity type, for instance. Since this task might not be trivial, this is another argument against using a field in this context.
I notice that the submodule does not have a README.md or .txt.
Should we add one, or put everything in the main module README?I think a README for the submodule would be fine.
- π©πͺGermany donquixote
I think a README for the submodule would be fine.
I changed my mind about this.
The existing README.txt already needs some work, e.g. to install it mentions Admin / Modules, when since D8 it is "Extend".
If we want to create a README.md or .txt in the submodule, we have to decide what goes there, and possibly change the main README.txt too.
In the spirit of smallest change possible, in this issue we can just add an UNINSTALL section to the existing README.txt.
- π©πͺGermany donquixote
donquixote β changed the visibility of the branch 3472140-remove-user-field-on-uninstall to hidden.