Problem/Motivation

Some classes and all interfaces in CRM are prefixed with "Crm", which goes against Drupal coding standards.

Per https://project.pages.drupalcode.org/coding_standards/php/coding/

Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace, read well, and are as short as possible without losing functionality information or leading to ambiguity

Steps to reproduce

Proposed resolution

Remove "Crm" from the class and interface names

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork crm-3557727

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jdleonard created an issue. See original summary.

jdleonard opened merge request !114

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented
Status: Active » Needs review
jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented
Assigned: jdleonard » Unassigned
bluegeek9’s picture

bluegeek9 commented

Interfaces are suppose to have the namespace prefixed to it, and end it Interface.

bluegeek9’s picture

bluegeek9 commented
Status: Needs review » Needs work
jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented
Status: Needs work » Needs review

@bluegeek9 agreed on Interface suffix. However, Drupal's coding standards states "interfaces should have names that stand alone to tell what they do without having to refer to the namespace".

bluegeek9’s picture

bluegeek9 commented

The Event class probably should be renamed.

Namespace Good name Bad names
Drupal\Core\Database\Query\ QueryCondition Condition (ambiguous)
DatabaseQueryCondition (Database doesn't add to understanding)
Drupal\Core\FileTransfer\ LocalFileTransfer Local (ambiguous)
Drupal\Core\Cache\ CacheDatabaseDriver Database (ambiguous/misleading)
DatabaseDriver (ambiguous/misleading)
Drupal\entity\ Entity
EntityInterface		 DrupalEntity (unnecessary words)
EntityClass (unnecessary words)
Drupal\comment\Tests\ ThreadingTest CommentThreadingTest (only needs to be unambiguous in comment context)
Threading (not ending in Test)

I recall drupal-check would flag interfaces that are not prefixed with the namespace as wrong. At some point there was an issue with running drupal-check in the pipeline, and I removed. Does drupal-check no longer mark it as a standards violation?

The author of drupal-check is ranked 15th

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented

"The Event class probably should be renamed." -- are you referring to "UserContactEvent"? What do you propose?

I propose that everything "UserContact" be renamed to "UserContactMapping" for clarity (so UserContactEvent would become UserContactMappingEvent), but that would probably be best as a follow up issue. Created #3558073: Rename "User Contact" to "User Contact Mapping"

I don't think what drupal-check flags is relevant if it doesn't match the current published coding standards.

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented

Almost forgot: consider https://www.drupal.org/project/hook_event_dispatcher in lieu of defining UserContactEvent

bluegeek9’s picture

bluegeek9 commented
Status: Needs review » Needs work
Issue tags: -CRM Beta Blocker

The custom event is not just because it is a hook. A custom event allows control over if the event if fired.

I do not want any additional hard dependencies.

The author of drupal-check is ranked 15th

If necessary for clarity or to prevent ambiguity, include the last component of the namespace in the name.

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented

I can understand not wanting to add dependencies. To be clear, my suggestion of Hook Event Dispatcher would be to eliminate the need to define a custom event; the conditional logic could be moved to the event listener. Regardless, that's a stylistic choice that I don't see as particularly important.

Can you please clarify whether you are stating that all the interfaces should be prefixed with "Crm" or what other work you feel is needed here?

I had marked this a beta blocker because changing class/interface names could break backward compatibility once we're in beta. Do you disagree?

bluegeek9’s picture

bluegeek9 commented

CrmRelationshipInterface, CrmContactInterface, and CrmContactDetailInterface are identical. They should be replace with a single CrmInterface.

bluegeek9’s picture

bluegeek9 commented

The other entity related namespaces can be pushed into the entity namespace and drop Crm.

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented
Status: Needs work » Needs review

There's value in not combining CrmRelationshipInterface, CrmContactInterface, and CrmContactDetailInterface as they might not remain identical forever. For example, if #3547287: Contact Detail Confirmation State were to result in some known states represented by strings, those might belong in CrmContactDetailInterface.

I pushed a refactor of the namespaces of various classes and interfaces. I matched the pattern used by the Group module, which has a similar level of complexity in the Entity namespace.

LMK what you think!

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented
Issue tags: +CRM Beta Blocker

Changing class and interface names is a backward compatibility break so re-tagging this as a beta blocker.

bluegeek9 changed the visibility of the branch 1.0.x to hidden.

bluegeek9’s picture

bluegeek9 commented
Status: Needs review » Needs work
Issue tags: -CRM Beta Blocker

There are too many changes.

Classes are being put into the wrong namespaces.

jdleonard’s picture

jdleonard
he/him
Primary language English
Location Austin, Texas
commented

Happy to work on this with some more guidance.

Which ones are wrong? Do you disagree with the namespace conventions used by the Group module? I couldn't find a coding standard that contradicted the conventions adopted by Group. I used that as a reference given the similar level of complexity.

Clearly you disagree that this is a beta blocker. I'd appreciate understanding why. I already explained my reasoning.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented

I'm also in favor of refactoring the classes this way.
Removing the "Crm" prefix for classes seems logical and avoids clutter in the filenames.

Moving the entity-related classes into specific "Entity/Access", "Entity/ViewBuilder", etc... namespaces also cleans up the unsorted single folder they were in previously.

This would need to be decided on fairly quickly I think, since it blocks other contributions (which would have merge conflicts).
Also better to change it early on, rather than refactoring later, if things have stabilized more.

bluegeek9’s picture

bluegeek9 commented
Assigned: Unassigned » bluegeek9
Status: Needs work » Active

bluegeek9 opened merge request !138

  • bluegeek9 committed c3ecfbc3 on 1.0.x 
    feat: #3557727 Align class and interface names with Drupal coding...

bluegeek9 opened merge request !139

  • bluegeek9 committed 01ce2227 on 1.0.x 
    feat: #3557727 Align class and interface names with Drupal coding...
bluegeek9’s picture

bluegeek9 commented
Assigned: bluegeek9 » Unassigned
Status: Active » Fixed
//www.flaticon.com/free-icons/thank-you Thank you for your contribution! Your continued support makes this project sustainable.
There are multiple ways to show appreciation for the work contributed to this project including:
  • Triage issues and adding more context to existing issues.
  • Flagging CRM as a favorite on the project page to help others discover it and show your support.
  • Review the Developer Docs for accuracy and clarity.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

bluegeek9 opened merge request !141

  • bluegeek9 committed 91e7b5d8 on 1.0.x 
    feat: #3557727 Align class and interface names with Drupal coding...

jdleonard closed merge request !114

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.