Problem/Motivation

This manifested itself in blowing up a kernel I was writing. Dependency I found came from the following service:

services:
  access_check.contact_personal:
    class: Drupal\contact\Access\ContactPageAccess
    tags:
      - { name: access_check, applies_to: _access_contact_personal_tab }
    arguments: ['@config.factory', '@user.data']

Proposed resolution

Add the dependency.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 2848728-contact-module-dependency-2.patch336 bytesSam152
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Sam152 CreditAttribution: Sam152 commented
Status: Active » Needs review
FileSize
2848728-contact-module-dependency-2.patch336 bytes
benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

LGTM

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

+1

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

Do we normally add dependencies on required modules?

[ibnsina:maintainer | Fri 17:46:28] $ grep -r "\- user" * | grep ".info.yml"
core/modules/basic_auth/basic_auth.info.yml:  - user
core/modules/block_content/block_content.info.yml:  - user
core/modules/contact/tests/modules/contact_storage_test/contact_storage_test.info.yml:  - user
core/modules/contact/tests/modules/contact_test_views/contact_test_views.info.yml:  - user
core/modules/filter/filter.info.yml:  - user
core/modules/system/tests/modules/entity_reference_test/entity_reference_test.info.yml:  - user
core/modules/system/tests/modules/token_test/token_test.info.yml: - user
core/modules/user/tests/modules/user_test_views/user_test_views.info.yml:  - user

I think a lot more things than that would depend on user...

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented

What says they're required? There isn't an assumption in the config system or the container and services that everything depends on user implicitly? It would make sense that without the dependency the issue would crop up in a cut-back KTB test.

Personally I think it's a good idea to make all dependencies explicit.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Closed (won't fix)

@xjm I had the same discussion with @Sam152 when he first faced the issue. I know @benjy is right in his observation but this is true as well that we add user module to almost all the KTB.

This is a special case just for user module and we have faced that issue a lot but I don't think just for the sake of tests we should fix this and yes we normally don't add dependencies on required modules. Let's add the module to dependent KTB and move on. Thanks!

Please feel free to open for more discussion.