Can't uninstall l10n_client_contributor

Created on 4 September 2024, 8 months ago

Problem/Motivation

While trying to translate a module locally, I fiddled a bit with this module. It looks like it doesn't work with Drupal 10... Now I can't uninstall the l10n_client_contributor anymore:

The following reason prevents Localization Client Contributor from being uninstalled:
The Localization client contributor API key field type is used in the following field: user.l10n_client_contributor_key

I didn't add any value to the user api key field.

No big problem, because it's only a test installation, but I wanted to let you know. I skimmed the commit history and it doesn't look like this issue was addressed since alpha2.

Steps to reproduce

Versions:

* Drupal 10.3.2
* drupal/xmlrpc 1.0.0-rc2
* drupal/l10n_client 3.0.0-alpha2 (because alpha3 and @dev both require Drupal 11)

Usage

* installed via composer
* enabled client via GUI
* tried, clicked, experienced bugs
* installed xmlrpc via composer (required by contributor)
* enabled contributor (in hope of magically fixing bugs)
* tried and clicked some more
* uninstalled client (worked)
* tried to uninstall contributor (failed)

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

πŸ› Bug report
Status

Active

Version

3.0

Component

Code

Created by

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • 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.

  • Pipeline finished with Failed
    16 days ago
    Total: 184s
    #470221
  • Pipeline finished with Failed
    16 days ago
    Total: 172s
    #470272
  • πŸ‡©πŸ‡ͺ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.

  • Merge request !15Add uninstall instructions. β†’ (Open) created by donquixote
  • Pipeline finished with Failed
    11 days ago
    Total: 145s
    #474161
Production build 0.71.5 2024