Problem/Motivation

CustomElementsFieldFormatterBase::getSetting() points into $this->configuration['settings']

But we (by which I mean: any formatters that are not wrapping Core Field formatters) are not using the 'settings' sub-array. So this is both bogus and confusing.

Intent

Not necessarily be perfect, but be harder to inadvertently call the wrong method, for people creating CeFieldFormatters.

Proposed resolution

Alternative 1:

Fix "bogus" by somehow moving the ['settings'] thing into a place where Core Field formatters are still using it, while CustomElementsFieldFormatterBase::getSetting() points into $this->configuration?

If that turns out to be impossible:

Fix "confusing" at least by adding more comments dissuading people from using getSetting(). Note we also have getConfiguration(), which is 'functionally duplicate'; point from one to the other. Maybe note (why) we don't implement PluginSettingsBase, if we haven't yet.

EDIT: This has turned out to be unnecessary, because CustomElementsFieldFormatterBase::getSetting() is not used by Core Field formatters at all. We can therefore just fix the "confusing" part, by removing ['settings'] from the getSetting() code.

Separate but related

Consider implementing a getSetting() equivalent that gets a single setting and returns the default value if it isn't set.

Either merge default config early, and audit the current formatters for any impacet... or (how PluginSettingsBase does it) merge it whenever getSetting() is called.

EDIT: default config is already being merged early. The problem is, we have defaultConfiguration() which is being merged, and defaultSettings() which isn't. This should be fixed by better code comments.

Issue fork custom_elements-3468343

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

roderik created an issue. See original summary.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Issue summary: View changes
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Issue summary: View changes
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Dumping draft comments I jotted down somewhere, to check at the same time. Not fixing for understandability - I will likely be the one to look at this anyway.

- take CoreFieldCeFieldFormatter
  - check buildConfigurationForm() & settingsForm().
  - what is what?						<-- settingsForm() is never used
  - when you understand that: fix the fact that CustomElementsFieldFormatterBase::settingsForm() / settingsSummary() have no definition in an interface AND STILL have @inheritdoc
    - then document that, in settingsform() (????), you should not use 'field_definition', 'view_mode', 'name', 'is_slot' -- because they will not be used.
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Issue summary: View changes

Better comments for myself / later reference (I will be able to find them back):

g/setSetting(s)() + static defaultSettings() are from PluginSettingsInterface

g/setConfiguration() + non-static defaultConfiguration() are from ConfigurableInterface

settingsForm() & settingsSummary() are not in an interface. Core field formatters have them in FormatterInterface but we don't implement that.

settingsForm() is not used anywhere, can be deleted. settingsSummary() we should probably add to CustomElementsFieldFormatterInterface. (We've done the same with static isApplicable() which ia also in FormatterInterface.)

build/validate/submitConfigurationForm() are from PluginFormInterface

There's a Core issue to merge PluginSettingsInterface into ConfigurableInterface (#1764380: Merge PluginSettingsInterface into ConfigurableInterface) but it isn't going anywhere. (If that is ever done, I assume all "*Setting*" names will be deprecated.)

The only reason why we implement PluginSettingsInterface, seems to be that EntityDisplayInterface:getRenderer()'s return value requires it. Otherwise we would not have these duplicate methods.

We could have chosen to not implement ConfigurableInterface instead, but we didn't. (Also PluginSettingsInterface::defaultSettings() is more clunky because static.)

roderik opened merge request !90

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Assigned: Unassigned » fago
Status: Active » Needs review

@fago for review, as this is a basic change to your code.

fago’s picture

fago
Primary language German
Location Vienna
commented

Thank you, this seems gresat! This improves things largely and will definitely help to avoid (further) confusion.

  • fago committed e3fa03ab on 3.x authored by roderik 
    Issue #3468343 by roderik: Doublecheck CustomElementsFieldFormatterBase...
fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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