Drupal\Core\Datetime\Entity\DateFormat class variables should not be accessed directly. Functions should be use to access the variable. For instance use getDescription() and setDescription($description) for the protected class variable description. For a boolean variable the getter function becomes isVariableName(). In object-oriented programming this is called encapsulation.

Remaining tasks

  • Update the class variables and make them protected.
  • Create getters and setters for frequently used get and set functionality.
  • Update drupal to use the getters and setters instead of accessing variables directly.
  • There are no tests required because the added functions are only getters and setters.

For more info over what should be done see the issue summary of #2016679: Expand Entity Type interfaces to provide methods, protect the properties.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because properties should not be public, API methods should not be allowed to be sidestepped.
Prioritized changes Prioritized since it is a bug and it reduces fragility.
Disruption Somewhat disruptive for core as well as contributed and custom modules:
  • BC break for anything using the public properties: code will need to convert to the methods
  • BC break for anything (mis)using properties that should not really be public: will require minor refactoring
  • BC break for alternate implementations of a given entity interface (rare/probably nonexistent): they will need to implement the new methods

But impact will be greater than the disruption, so it is allowed in the beta.

CommentFileSizeAuthor
#1 2527076-1.patch598 bytesdaffie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review
FileSize
2527076-1.patch598 bytes

Setting the two class variables that are not protected to protected. The variable are $id and $label.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good. As the id() and label() are already defined in parent class, we can still get the value of these properties.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Talked to catch on IRC and this issue can go in before 8.0. Only there is a change record necessary.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Change record added. Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Given that this is the last set of public properties on a config entity that really shouldn't be I think that this is okay to proceed with at this point in the beta.

Committed dfa5621 and pushed to 8.0.x. Thanks!

  • alexpott committed dfa5621 on 8.0.x 
    Issue #2527076 by daffie: Make the class variables protected for Drupal\...

Status: Fixed » Closed (fixed)

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