Enh: harmonise term enabled/disabled vs active/inactive for modules

[martin-rueegg]

Renaming thought the code (of core) the terms that relate to Module's enabled state:

• active => enabled
• activate => enable
• deactivate => disable

PR Admin

What kind of change does this PR introduce?

• Code style update
• Refactor

Does this PR introduce a breaking change?

• Yes

If yes, please describe the impact and migration path for existing applications:

documented in MIGRATE-DEV.md

The PR fulfills these requirements

• It's submitted to the develop branch, not the master branch if no hotfix
• When resolving a specific issue, it's referenced in the PR's description (e.g. Fix #xxx[,#xxx], where "xxx" is the Github issue number)
• All tests are passing
• New/updated tests are included
• Changelog was modified

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

See

• Enh improve module migrations #6550 (comment) (@luke-)

  • I like the idea to remove the term active and always work with enabled. If you like feel free to create a PR and mark activate* as deprecated for now. @yurabakhtin ok from your side?

  Turns out, \humhub\modules\friendship\Module::getIsEnabled() is used for a different purpose. Shall we rename this or does that cause too much ripple?

  Hmm, I think we can rename it. Any idea? isActivated? :-)

• Enh improve module migrations #6550 (comment) (@luke-)

  Otherwise, I'd use a more specific term, e.g. isFriendshipEnabled() (I'd not use "active" as the functionality may be "active" and still no one uses them, so there is no "active" friendship. "enabled" or "available" would reflect the potentiality.

  For now I would prefer isFriendshipEnabled(). The friendship module is quite special. It's a core module, always activated, but can be disabled via Admin Settings.

• Enh improve module migrations #6550 (comment)

  @luke-
  There are also a few places, where the term enabled or isEnabled is used

  • \humhub\modules\file\models\FileHistory::isEnabled() refers to the functionality, not the module (should be fine)
    👍

  • \humhub\modules\marketplace\Module::isEnabled() connected to
  • \humhub\modules\marketplace\Module::$enabled which is set to false if Yii::$app->params['humhub']['apiEnabled'] is not true, and could also be disable through configuration

  TBD

  • \humhub\modules\content\components\ContentContainerModuleManager::isEnabled($id) checks if module $id is enabled on the container

  Is ok or?

  So the there is some potential for confusion between

  • \humhub\modules\marketplace\Module::isEnabled(), which refers to a configured state, and it's inherited
  • \humhub\components\Module::getIsEnabled(), which refers to the module enabled state (but which is always true for a core module like the marketplace.

  Would it make sense to also rename \humhub\modules\marketplace\Module::isEnabled() into \humhub\modules\marketplace\Module::getIsEnabled(), overriding the \humhub\components\Module::getIsEnabled()? In a way it seems to have a similar meaning. Or does that increase the confusion?
  Alternatively, we could obviously leave it, as it is, or rename it to \humhub\components\Module::isMarketplaceEnabled(), analogous to the friendship module.

  Hmm, I would prefer to introduce \humhub\modules\marketplace\Module::isMarketplaceEnabled() analgour to the new \humhub\modules\friendship\Module::isFriendshipEnabled(). What do you think @yurabakhtin ?

  For Module::isFriendshipEnabled() we need to do some refactorings in the calendar, cfiles, content-bookmarks, mail , task.

[martin-rueegg]

Have rebased on top of develop and fixed the merge conflict.

[luke-]

@martin-rueegg Thanks!

We should also prepare PRs for the following modules that check whether the friendship system is enabled.

• Calendar
• CFiles
• ContentBookmarks
• Tasks
• Mail

Did I understand correctly, that \humhub\components\Module::getIsEnabled() is always present, thus this change does not cause a fatal error on modules. But unmigrated modules always detect the friendship module as enabled. (Core Module).

[luke-]

I couldn't find any other uses of isActivated in our modules.

@yurabakhtinDo you have any ideas where there is a need for migration?

[yurabakhtin]

I couldn't find any other uses of isActivated in our modules.

@yurabakhtinDo you have any ideas where there is a need for migration?

@luke- I could not find where isActivated is used in modules.

[martin-rueegg]

Did I understand correctly, that \humhub\components\Module::getIsEnabled() is always present, thus this change does not cause a fatal error on modules.

Yes, that's correct, as long as the module extends from \humhub\components\Module::getIsEnabled(), which is certainly true for the friendship module - if not for all modules.

But unmigrated modules always detect the friendship module as enabled. (Core Module).

Yes, I guess also this is true as the friendship module is enabled by default and cannot be disabled either.

[martin-rueegg]

I couldn't find any other uses of isActivated in our modules.
@yurabakhtinDo you have any ideas where there is a need for migration?

@luke- I could not find where isActivated is used in modules.

This may be a no-brainer. But it is importent to check both

• field module::isActivated, and
• method module::getIsActivated()

Although, also here no fatal error is expected, as module::getIsActivated() has only been deprecated, not removed.

[luke-]

Ok great, then the migration effort only affects the modules which checks for enabled friendship: #6745 (comment)

[martin-rueegg]

@luke- what is currently holding back this PR?

[luke-]

@martin-rueegg Missing PR's for the modules listed here: #6745 (comment)

Besides that, the PR looks good for me and is ready to merge!

[martin-rueegg]

We should also prepare PRs for the following modules that check whether the friendship system is enabled.

• Calendar
  [v1.16] Update method of checking friendship humhub/humhub#6745 calendar#442
• CFiles
  [v1.16] Update method of checking friendship humhub/humhub#6745 cfiles#200
• ContentBookmarks
  No usage found
• Tasks
  [v1.16] Update method of checking friendship humhub/humhub#6745 tasks#248
• Mail
  No usage found

[martin-rueegg]

@luke- I've created some PRs to the best of my knowledge. I'm not sure, if older HH versions should be supported, in which case a version check would have to be made before calling the method.

The tests of cours fail, as this current PR has not yet been merged.

[luke-]

@martin-rueegg Thanks for the PRs. We'll merge them and release a module version shortly before the v1.16 release.

[luke-]

@martin-rueegg FYI, I've reverted the UI wording changes. We need to discuss this internally.

[martin-rueegg]

@martin-rueegg Thanks for the PRs. We'll merge them and release a module version shortly before the v1.16 release.

Thanks for your info. I guess it would make most sense that the two terms are identical. Hence, if you decide to keep "enable/disable" in the UI, maybe the same should be the case for the code.

Either way, the adjective "successful" in line https://github.com/humhub/humhub/pull/6745/files#diff-ac4e86a568e376b4163bc52621367fad02dfdf2341771df1ed39132d7ecea53cR24 should still be an adverb (successfully). :-)