Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods

Here is a first attempt.

#1: Expand_field_instance.patch queued for re-testing.

Postponing this on #1953408: Remove ArrayAccess BC layer from field config entities

Posponed on #2095303: Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config' now

Attempted re-roll of #1 with some modifications to respect the current codebase.
Hope I did not miss anything.

10: expand-field-instance-2030637-10.patch queued for re-testing.

I get the following error on a running drupal install:

Will try to do backtrace.

Additional uncaught exception thrown while handling exception.
Original

Drupal\Core\Config\StorageException: Missing configuration file: system.site in Drupal\Core\Config\InstallStorage->getFilePath() (line 62 of W:\domains\localhost\core\lib\Drupal\Core\Config\InstallStorage.php).

Drupal\Core\Config\InstallStorage->getFilePath('system.site')
Drupal\Core\Config\FileStorage->exists('system.site')
Drupal\Core\Config\FileStorage->read('system.site')
Drupal\Core\Config\FileStorage->readMultiple(Array)
Drupal\Core\Config\ConfigFactory->loadMultiple(Array)
Drupal\Core\Config\ConfigFactory->get('system.site')
Drupal::config('system.site')
drupal_match_path('', 'admin
admin/*
batch')
path_is_admin('')
_drupal_add_js(Array, Array)
call_user_func('_drupal_add_js', Array, Array)
drupal_process_attached(Array, 1)
_drupal_add_library('core/drupalSettings')
drupal_process_attached(Array, 1)
_drupal_add_library('core/drupal.states')
drupal_process_attached(Array)
drupal_render(Array, 1)
drupal_render(Array)
install_display_output(Array, Array)
install_drupal()

Additional

Drupal\Core\Config\StorageException: Missing configuration file: system.theme.global in Drupal\Core\Config\InstallStorage->getFilePath() (line 62 of W:\domains\localhost\core\lib\Drupal\Core\Config\InstallStorage.php).

Drupal\Core\Config\InstallStorage->getFilePath('system.theme.global')
Drupal\Core\Config\FileStorage->exists('system.theme.global')
Drupal\Core\Config\FileStorage->read('system.theme.global')
Drupal\Core\Config\FileStorage->readMultiple(Array)
Drupal\Core\Config\ConfigFactory->loadMultiple(Array)
Drupal\Core\Config\ConfigFactory->get('system.theme.global')
Drupal::config('system.theme.global')
theme_get_setting('features.favicon')
template_preprocess_maintenance_page(Array)
template_preprocess_install_page(Array, 'install_page', Array)
_theme('install_page', Array)
drupal_render(Array)
install_display_output(Array, Array)
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)

That backtrace is not very useful.

This is the actual error:

Fatal error: Cannot access protected property Drupal\field\Entity\FieldInstanceConfig::$id in /Users/drupal/drupal/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php on line 324
Drush command terminated abnormally due to an unrecoverable error.                                                                                                        [error]
Error: Cannot access protected property Drupal\field\Entity\FieldInstanceConfig::$id in
/Users/drupal/drupal/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php, line 324

Which is very normal as the config storage controller now tries to access a protected property. This won't work at all. Furthermore, if you start 'fixing' this, you'll encounter more direct property access in many other files. So we also need setters too, for instance for 'settings' and such. This won't be an easy one to get right.

Also not sure how useful the getId() method is, this should rather live on configEntityBase I think.

Sorry for my last post, this bug I have with fresh git version, no patches applied :(

Hm. Apparently the ConfigStorageController.php does:

  // Build an ID if none is set.
  if (!isset($entity->{$this->idKey})) {
    $entity->{$this->idKey} = $entity->id();
  }

From #2054699: Ensure config entity id is set for computed composite ids.
I also just searched for some ConfigTyps that extend ConfigEntityBase: CustomBlock, Block, Breakpoint, BreakpointGroup, Contact
All those seem to implement a custom $id property.

But ConfigEntityBase also has an $originalId property which holds exactly the same info, though it seems to be for a different use-case:

  /**
   * The original ID of the configuration entity.
   *
   * The ID of a configuration entity is a unique string (machine name). When a
   * configuration entity is updated and its machine name is renamed, the
   * original ID needs to be known.
   *
   * @var string
   */
  protected $originalId;  
  {...}
  // Backup the original ID, if any.
  // Configuration entity IDs are strings, and '0' is a valid ID.
  $original_id = $this->id();
  if ($original_id !== NULL && $original_id !== '') {
    $this->setOriginalId($original_id);
  }

Actually all config-entitys have an $id property, which could be could be, but must not is reems reasonable to move that property to ConfigEntityBase. This it must not be that way judging form the code above as the id key is generated like this:
$this->idKey = $this->entityType->getKey('id');

Also there is:
#2016679-14: Expand Entity Type interfaces to provide methods, protect the properties
#2016679-18: Expand Entity Type interfaces to provide methods, protect the properties
#2016679-29: Expand Entity Type interfaces to provide methods, protect the properties

We should propably leave the properties public as suggested in #2016679-29: Expand Entity Type interfaces to provide methods, protect the properties and also done in nearly all other issues working on ConfigEntities.

In that case it's only getters for:

• FieldInstanceConfig::$field_name
• FieldInstanceConfig::$field_uuid
• FieldInstanceConfig::$entity_type
• FieldInstanceConfig::$label
• FieldInstanceConfig::$description
• FieldInstanceConfig::$settings
• FieldInstanceConfig::$required
• FieldInstanceConfig::$default_value
• FieldInstanceConfig::$default_value_function
• FieldInstanceConfig::$deleted

And setters where applicable:

Check if it makes sense

Only add setters if it makes sense conceptually: setId() is not necessary for database/content entities, as the ID is automatically generated for them when they are saved. Another example: setUUID(), this is set automatically initially and must not be changed afterwards.

right?

These getter exist already:

• FieldInstanceConfig::$entity_type => getTargetEntityTypeId()
• FieldInstanceConfig::$label => getLabel()
• FieldInstanceConfig::$description => getDescription()
• FieldInstanceConfig::$settings => getSettings() AND getSettings($settings_name)
• FieldInstanceConfig::$required => isRequired()
• FieldInstanceConfig::$default_value => getDefaultValue()
• FieldInstanceConfig::$default_value_function => getDefaultValue()

With this I'm unsure:

• FieldInstanceConfig::$field_name => getName() : though it accesses $this->field->name

For these there is still a getter missing:

• FieldInstanceConfig::$field_uuid => is this the same one as uuid() ?
• FieldInstanceConfig::$deleted => isDeleted() ?

This setter could be resonable:

• setLabel() => Dont have a proper Idea for this as the getter returns $this->label();
• setDescription()
• setSettings() => Dont have a proper Idea for this as the getter returns $this->settings + $this->field->getSettings();
• setSettings($settings_name)
• setDeleted() => absolutely unsure about this

Does this make any sense or did I miss something?

This patch starts from scratch and changes FieldInstanceConfig and FieldInstanceConfigInterface by

adding getters for:

• FieldInstanceConfig::$field_uuid => though it calls $this->field->uuid(); to ensure that the correct uuid is used
• FieldInstanceConfig::$deleted

adding setters for:

• FieldInstanceConfig::$settings => though only setSettings($settings_name, ..) to change instance level settings
• FieldInstanceConfig::$label
• FieldInstanceConfig::$description
• FieldInstanceConfig::$required => though the getter for this is on FieldDefinitionInterface

changes getter for:

• FieldInstanceConfig::$label => Based on constructor code, the field instance should be allowed to change the field label for the current instance. The getter now returns the label propery to reflect that.

I think the folders structure is changed.
Patch needs to be changed.
Currencly file core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php doesnt exist as well as the core/modules/field/lib/Drupal/field/Entity folder.

Re-roll of #24.

+++ /dev/null
@@ -1,53 +0,0 @@
-parameters:

--- a/sites/default/default.settings.php
+++ b/sites/default/default.settings.php

unrelated to patch

Based on testbot: Merge failed on default.settings.php and default.services.yml. Pulled those files from HEAD and here's the new patch.

Testbot passes because we patch classes which are deleted by the rebase.

The patch only introduces an interface and then doesn't add tests.

This might be a won't fix issue....

Updating title for the new name of the config entity.
+ meanwhile, BaseFieldOverride got added (both entity types have the same content and mostly inherit from the same FieldConfigBase base class, so they are best handled together)

The class BaseFieldOverride needs an interface. The following function are new in the class: createFromBaseFieldDefinition() and loadByName().

The current patch introduces an interface for a class that already has an interface. So the patch no longer applies.

The class FieldStorageConfig has a lot of public variables that need to be changed to protected.

BaseFieldOverride extends FieldConfigBase, which implements FieldConfigInterface, and through inheritance, ConfigEntityInterface. I doubt BaseFieldOverride needs its own interface.

I did this: #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface

I think that issue is blocking this one, and probably others in #2016679: Expand Entity Type interfaces to provide methods, protect the properties

Thank you @Mile23 I really hope we can get these issues done before release, and this found something very tricky.

--

Removing Novice tag.
The novice parts of this have already been done, now the work is trickier.
https://drupal.org/core-mentoring/novice-tasks

--

in
#2398901-16: Image module makes fatal assumptions about EntityInterface
@yched suggests 3 ways forward.

I'm in favor of:

b) Keep separate patches, and each patch temporarily adds setSettings() to the other interface.
Minor overlap, perfectly acceptable, will just mean a reroll when the first one gets in.

--

Unassigning so it is clear anyone can try to do that.

Merged this issue with #2030633: Expand FieldStorageConfig with methods. The reason why is explained in this comment. I gave it the status of "Closed (won't fix)". I that is wrong please correct me.

Are you sure about this? I already posted a patch in #2030633: Expand FieldStorageConfig with methods that makes it not necessary to merge it, this will simply have to clean up 3 lines of code then. The patch there is already pretty large, not sure if we should make it bigger.

On #2030633: Expand FieldStorageConfig with methods.

I'm not sure it is postponed either. I think we can work on this, and reroll if necessary if the other issue gets committed first.

Patch in #31 still applies. Setting it to re-test.

None of the others are committed, or really even reviewed.

Oh yeah, that was the patch that just added an interface. Doh. Let's start over. :-)

This patch basically ignores all previous patches, which have been refactored away in other issues.

So at this point BaseFieldOverride is already encapsulated (in that it doesn't have any public properties).

FieldConfig has only one public property, $deleted, which is already encapsulated as isDeleted(). So I'll just change that to be protected and whatever other incompatibilities arise from that.

It all looks good to me.
All the variables are set to protected.
All documentation is in order.
It get a RTBC from me.

So what about fieldConfigBase? I think this issue should be expanded in scope to cover that to because it is the shared base class of FieldConfig & BaseFieldOverride

Needed a reroll.

What alexpott said in #48.

I expect that this adds a setSetting() method, among other things. so that we can simplify image_entity_presave() from

  // @todo Simplify this in https://www.drupal.org/node/2030637, when both
  //   interfaces have a setSetting() method.
  if ($entity instanceof FieldStorageConfigInterface) {
    $entity->setSetting('default_image', $default_image);
  }
  elseif ($entity instanceof FieldConfig) {
    $entity->settings['default_image'] = $default_image;
  }

To

$entity->setSetting('default_image', $default_image);

Because we know that both interfaces/entity types that we are working with have that method.

The concerns in #50 will be met by any work on FieldConfigBase, because that's where we'd implement setSetting().

However, work on FieldConfigBase for #48 will be major, and will conflict with this critical: #2416409: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted

Filed a follow-up here: #2426647: Expand FieldConfigBase with methods which is postponed on the critical.

Rather than creating a new postponed issue I suggest that we continue here - just as long as we don't do anything with the noFieldDelete property. And I don't mind rerolling the critical - which is postponed as well.

alexpott wants to expand this issue to also cover fieldConfigBase class (see comment #48).

Added the fieldConfigBase class to the patch.
All class variables are set to protected, accept for $default_value.

Fixed some mistakes in the patch and updated the issue summary.

Reroll

Reroll for now.

Then, fixing FieldConfigEditForm, that got added meanwhile.

- Removed the getBundle() method, it duplicates the existing getTargetBundle() we already have from FieldDefinitionInterface.
- Fixed a couple tests (i.e chasing HEAD since the last green patch)

Should be green.

Just reordered the methods a bit (getters before setters), and made the ordering consistent between FieldConfigInterface and FieldConfigBase.

Then I actually reviewed the changes (at last...)

I'm wondering :
- why setRequired() is not added to FieldConfigInterface like the other setters added there, but to FieldDefinitionInterface above ?
It's a fact that FDI and FSDI lack setters, but IIRC we have an issue for it, meanwhile I'd rather be consistent in our inconsistency :-/
- why FieldConfigBase::$default_value is the only property that remains public ?

Will post a patch that fixes the above, and see what fails

Sorry, first I forgot to upload the patch for my "Just reordered the methods a bit" comment above :-)

Then, as per #67, checking what happens if
- setRequired() is added to FieldConfigInterface rather than FieldDefinitionInterface
- FieldConfigBase::$default_value goes protected

Er, d.o, you must be kidding me.

OK, FieldConfigBase::$default_value is tricky because we don't have a getter for "the raw array stored in the property", only for "assemble the default value to use at runtime on newly created entities, can come from FieldConfigBase::$default_value or FieldConfigBase::$default_value_callback".
And there is some code that needs to do housekeeping on the former (EntityReferenceItem::calculateDependencies() for example)

I think we have an issue for that somewhere :-/

Reverting that then.

OK, this is ready for me then - I guess this will need someone else to push the RTBC button though :-/

Also, bumping to major, since the sister patch in #2030633: Expand FieldStorageConfig with methods got in, it would be pretty inconsistent to ship that way.

Rest assured that the irony is not lost on me, but - my turn to bump and beg for an RTBC :-)

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
   @@ -160,25 +160,6 @@ public function isDisplayConfigurable($display_context);
   -  public function isRequired();
   

   I don't see anywhere in this issue why we remove this? The method does exist on the parent, but it seems we're removing possibly relevant documentation here?

2. +++ b/core/modules/field_ui/src/Form/FieldConfigEditForm.php
   @@ -35,18 +35,20 @@ class FieldConfigEditForm extends EntityForm {
   -    $field_storage = $this->entity->getFieldStorageDefinition();
   +    /** @var \Drupal\field\Entity\FieldConfig $field */
   +    $field = $this->entity;
   +    $field_storage = $field->getFieldStorageDefinition();
   
   @@ -55,7 +57,7 @@ public function form(array $form, FormStateInterface $form_state) {
          '#title' => $this->t('Label'),
   -      '#default_value' => $this->entity->getLabel() ?: $field_storage->getName(),
   +      '#default_value' => $field->getLabel() ?: $field_storage->getName(),
   

   This seems to result in quite a few changes that aren't really related to this issue?

   What we sometime do instead is document the type by overriding protected $ntity at the top of the class.

Other than that, looks ready to me.

Thanks @Berdir !
And damn, busted on unrelated changes :-p

1. Well, this does add information about empty items being filtered out before checking validation :

   * If a field is required, an entity needs to have at least a valid,
   * non-empty item in that field's FieldItemList in order to pass validation.
   *
   * An item is considered empty if its isEmpty() method returns TRUE.
   * Typically, that is if at least one of its required properties is empty.

- Never been a fan of an interface overriding another interface just to add a different phpdoc :-)
- Those two paragraphs are already present and exactly duplicated in the phpdoc for FieldConfigInterface::setRequired(), where they arguably belong best (to inform the decision of setting to required or not). We usually avoid duplicate docs...
- But right, not really related :-). We can clean that up (or not) in the issue where we move setters in the FieldDefinitionInterface / FieldStorageDefinitionInterface, if we ever get there ...

2. Right, not really related either, it's just that all those $this->entity->something() get a bit old. But well apparently I didn't even consistently replaced all of them :-). Reverted to the current $this->entity.

Sorry :)

1. Would have been OK with that explanation (which makes sense), just wasn't sure if that was accidental or not.

Committed f8f024e and pushed to 8.0.x. Thanks!

Committing under the maintainer discretion for fragility as per the consistency argument made in #75.

Awesome. Thanks a lot @all that kept pushing this, and sorry that it took me so long to get to it

The issue summary for this is not accurate. The patch that was committed adds new methods, but it also changed the scope for some class properties from public to protected, which is an API change. At the very least, this needs a change record.

probably update of CRs is better

There is still the variable FieldConfigBase::default_value that needs to be set to protected. I know that is a difficult one and in the ycheds words (#74):

OK, FieldConfigBase::$default_value is tricky because we don't have a getter for "the raw array stored in the property", only for "assemble the default value to use at runtime on newly created entities, can come from FieldConfigBase::$default_value or FieldConfigBase::$default_value_callback".

Created a followup issue: #2529034: Replace direct access to FieldConfigBase::default_value with methods.

Only a change record is still needed.

Opened another followup #2589829: Missing return $this in FieldConfigBase::setSetting and setConstraints

This issue was committed to 8.0.x and re-opened to add a change record. Since that was for Drupal 8.0, which is now End of Life, this is now outdated.

If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!

I have come to this issue during a bugsmash group triage on #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface.

I incorrectly set the status to outdated. It should be Fixed and moved to 8.0.x, per the commit on 8.0.x. Adding credit for the work done after the commit.