Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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:
- Make class members of configuration entities that map to configuration keys
underscore_separated.
Con: This violates the coding standards for object-oriented code. - Make configuration keys that map to class members of configuration entities
lowerCamelCase.
Con: This violates the coding standard for configuration keys. - Change our coding standard to require
lowerCamelCasefor all configuration keys.
Con: It feels unnatural to uselowerCamelCasefor non-object-oriented code. (This in a way applies to 2. as well.) - Introduce an abstraction layer that maps
underscore_separatedconfiguration keys tolowerCamelCaseclass members.
Con: More code, additional abstraction.
User interface changes
-
API changes
-
Comments
Comment #1
tstoecklerSorry, I guess should be documentation.
Comment #2
attiks CreditAttribution: attiks commentedIf 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.
Comment #3
jhodgdonWow, interesting conundrum!
Can you add to your issue summary: links to the two competing coding standards that affect this? Thanks!
Comment #4
Crell CreditAttribution: Crell commentedAs 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... :-) )
Comment #4.0
tstoecklerUpdated issue summary.
Comment #5
tstoecklerUpdated 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:
(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:
I.e. ideally we would have some sort of abstraction layer anyway, and at that point whether we return
$this->somePropertyor$this->config['some_property']is minor anyway.Comment #6
tim.plunkettOr, we just run everything through Container::camelize(), which is already used for bundle naming.
Comment #7
Crell CreditAttribution: Crell commentedActually I was saying that Entity API went with option 1.
I am not necessarily endorsing that approach, nor condemning, just providing background.
Comment #8
jhodgdonI 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?
Comment #9
gddWe 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.
Comment #9.0
gddUpdated issue summary. Fix markup and move options to "Remaining tasks"
Comment #10
bojanz CreditAttribution: bojanz commentedTwo 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.
Comment #11
dawehner.
Comment #12
bojanz CreditAttribution: 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.
Comment #13
bojanz CreditAttribution: bojanz commentedNot following our own coding standards is a clear consistency issue, marking it as such.
Comment #18
Chi CreditAttribution: Chi commentedComment #21
andypostComment #30
Wim LeersIs this really a problem? 🤔 Tempted to close this 😇