Problem/Motivation

The member variables of ConfigEntity's map 1-to-1 to the configuration keys in the corresponding config YAML files. Coding standards dictate lowerCamelCase for member variables in OOP code and underscore_separated for config keys. We need to decide what we want here. Most ConfigEntity's are not bitten by this, because they only contain one-word keys, in which case lowerCamelCase and underscore_separated fall together. :-)

We already have an inconsistency in core with the View entity (View::base_table) and the Breakpoint and BreakpointGroup entities.

Proposed resolution

-

Remaining tasks

Decide on one of the following:

  1. Make class members of configuration entities that map to configuration keys underscore_separated.
    Con: This violates the coding standards for object-oriented code.
  2. Make configuration keys that map to class members of configuration entities lowerCamelCase.
    Con: This violates the coding standard for configuration keys.
  3. Change our coding standard to require lowerCamelCase for all configuration keys.
    Con: It feels unnatural to use lowerCamelCase for non-object-oriented code. (This in a way applies to 2. as well.)
  4. Introduce an abstraction layer that maps underscore_separated configuration keys to lowerCamelCase class members.
    Con: More code, additional abstraction.

User interface changes

-

API changes

-

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Component: entity system » documentation

Sorry, I guess should be documentation.

attiks’s picture

attiks commented

If you run coder on your config entity class is says it should be camelCase, that's way breakpoint is using it. But you're absolutely right, we need a consensus.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Decide on coding standard for configuration keys of ConfigEntities » [policy, then patch] Decide on coding standard for configuration keys of ConfigEntities
Issue tags: +Coding standards

Wow, interesting conundrum!

Can you add to your issue summary: links to the two competing coding standards that affect this? Thanks!

Crell’s picture

Crell commented

As a data point, I believe when we were discussing the Entity API and the same question came up (properties should be camelCase, but DB fields are lower_case), the decision was "properties that map 1:1 to DB tables *and* are dynamic should follow the DB table, but properties defined on the class should be camelCase".

I'm not sure what's happened to Entity API since, but that's my memory of that conversation. (That and encouraging people to not have direct 1:1 table mappings anyway because it breaks abstraction... :-) )

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue summary: View changes

Updated issue summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Updated the issue summary to contain links to current code and to mention the possible solutions.

Thinking about it, I really like 4. (which is what I think @Crell meant in #4, but I'm not sure). For consumers of the API this could be as easy as:

class SomeConfigEntity {

  ...

  protected or perhaps public static $mapping = array(
    'some_configuration_key' => 'someConfigurationKey',
    'some_other_configuration_key' => 'someOtherConfigurationKey',
    'foo_xml_bar_rss_baz_xss' => 'fooXMLBarRSSBazXSS',
  );

}

(I don't think we can automate such a mapping because of the third key, for example.)

It would be a little more work in the storage controller, but not much.

Also - and maybe I'm going nuts here - once we have proper interfaces for all entities, we could do:

class SomeConfigEntity {

  ...

  /**
   * Implements SomeConfigEntityInterface::someProperty()
   */
  public function someProperty() {
    return $this->config['some_property'];
  }

}

I.e. ideally we would have some sort of abstraction layer anyway, and at that point whether we return $this->someProperty or $this->config['some_property'] is minor anyway.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Or, we just run everything through Container::camelize(), which is already used for bundle naming.

Crell’s picture

Crell commented

Actually I was saying that Entity API went with option 1.

I am not necessarily endorsing that approach, nor condemning, just providing background.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I know where the coding standard is that says "all methods and properties should be lowerCamelCase": http://drupal.org/node/608152

Where is the coding standard about configuration though? And could you put a link to both standards in the issue summary please?

And re #6, Container::camelize() will not work exactly with our camel standards (which require things like XML and HTML to be all-caps) I think? Minor problem though... I am also not sure what it would mean to "run everything through camelize" anyway -- when? where? how would this work?

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

We don't have a coding standard for config values, although I can and probably should write one up. Basically we've been using the same standards as we do for PHP variables.

I would agree with Crell that we should proceed with #1. I believe all the existing config entities have already been implemented with this standard, and it might serve as a visual cue that these properties serve a somewhat different purpose than others.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Issue summary: View changes

Updated issue summary. Fix markup and move options to "Remaining tasks"

bojanz’s picture

bojanz commented
Priority: Normal » Major
Issue summary: View changes
Issue tags: +DX

Two years later, from a contrib perspective this is very confusing and very annoying.

The situation is not the same as for content entities, because content entities don't declare the properties, so they don't break the coding standards the same way config entities do.
Luckily, config entities now have mandatory getters and setters, which means that we can fix the property names without breaking BC.

Assumptions:
- We want to keep the snake_case config keys, preserving consistency with plain config files
- We want to have camelCase properties on the class, preserving consistency with other classes in the system, and respecting the coding standards.

We can do this with minimum effort by mapping the snake_case config keys to the camelCase properties.
While we can do it automatically (Container::camelize() and a new function that works in reverse), it is potentially error prone, and might have a performance impact.

So I suggest having a defined mapping for each config entity, as a part of the annotation. The mapping would only be used if found, allowing us to fix config entity types one by one. Thoughts?

EDIT: So to clarify, when loading the config data into an entity, the flow is schema -> mapping -> set.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: -DX +DX (Developer Experience)

.

bojanz’s picture

bojanz commented

#2411967: Allow config entities to specify the config key <-> entity property mapping now has a patch demonstrating my #10 proposal.

EDIT: alexpott is doing a similar solution in #2432791: Skip Config::save schema validation of config data for trusted data.

bojanz’s picture

bojanz commented
Issue tags: +consistency
Related issues: +#2471679: [Meta] Make core internally consistent

Not following our own coding standards is a clear consistency issue, marking it as such.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chi’s picture

chi commented
Related issues: +#2530922: Config Entities need an exception from $camelCase properties

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.6.x-dev » 8.8.x-dev

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Configuration schema

Is this really a problem? 🤔 Tempted to close this 😇

quietone’s picture

quietone commented
Project: Drupal core » Coding Standards
Version: 11.x-dev »
Component: documentation » Coding Standards
quietone’s picture

quietone commented
Title: [policy, then patch] Decide on coding standard for configuration keys of ConfigEntities » Decide on coding standard for configuration keys of ConfigEntities
Priority: Major » Normal
Status: Active » Postponed (maintainer needs more info)

We now have this in the related coding standard is

Methods and class properties should use lowerCamel naming. In Drupal 8, properties of configuration entities are exempt of these conventions. Those properties are allowed to use underscores.

And the sniff Drupal.NamingConventions.ValidVariableName.LowerCamelName is enabled. The exceptions are ignore like this

  // phpcs:ignore Drupal.NamingConventions.ValidVariableName.LowerCamelName
  protected $migration_tags = [];

I am inclined to agree with @Wim Leers, that this can be closed. Anyone disagree?

quietone’s picture

quietone commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

There hasn't been any response to indicate that further work is required here. Therefor I am closing this as outdated.