Warning: This issue is dependent on, and caused by, issue #2030633: Expand FieldStorageConfig with methods

Problem

In an ongoing effort to provide classes/interfaces with methods that get and set their properties, see #2016679: Expand Entity Type interfaces to provide methods, protect the properties, the FieldConfig class was expanded to have getters and setters, see #2030633: Expand FieldStorageConfig with methods. However, there are still an unknown number of references directly to the public properties that should be updated to use these new methods.

One way to see where the methods are directly called is to set the properties to be protected and see what blows up. This was done in comment 39 of #2030633: Expand FieldStorageConfig with methods. The test bot will fail at the first instance so a better approach to testing should be found rather than repeatedly submitting intentionally broken patches.

Proposed resolution

Find places in the code that call the properties directly and update them to use the methods instead. For example, the ContentEntityDatabaseStorage class references the $field->deleted property directly instead of using the $field->isDeleted() method.

Remaining tasks

The magnitude of this issue is not yet known. Someone should figure out how to run the full test suite against code that has the FieldConfig properties set to protected to see the extent of the problem.

CommentFileSizeAuthor
#5 2282299_AccessingFieldConfig.patch23.3 KBacrosman

Comments

carsonevans’s picture

carsonevans commented
Issue summary: View changes
carsonevans’s picture

carsonevans commented
Related issues: +#2030633: Expand FieldStorageConfig with methods
cilefen’s picture

cilefen commented

One can search the source code for likely usages with something like this:

grep -rl "use Drupal\\\field\\\Entity\\\FieldConfig" core/ |xargs grep -i "\\->cardinality"

Where cardinality is one of the soon-to-be-former public properties such as:

id, cardinality, name, entity_type, type, module, settings, translatable, locked, indexes, deleted

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented
Assigned: Unassigned » acrosman

Should be able to find all the effected lines with something along the lines of:
\$field->(id|cardinality|name|entity_type|type|module|settings|translatable|locked|indexes|deleted)(?!\(\))

My review comes back with something on the order of 25 references across the core directory.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented
StatusFileSize
new 2282299_AccessingFieldConfig.patch23.3 KB

This patch should resolve all the references I found. It assumes the patch from comment 64 on #2030633: Expand FieldStorageConfig with methods has been applied first.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2282299_AccessingFieldConfig.patch, failed testing.

swentel’s picture

swentel commented 
+++ b/core/modules/views/views.api.php
--- a/sites/default/default.settings.php
+++ b/sites/default/default.settings.php

Looks like this also contains another patch that shouldn't be here.

daffie’s picture

daffie commented
Issue tags: +Needs reroll

The class variable "deleted" is now public and needs to be changed to protected. We want to use the principle of encapsulation.

yesct’s picture

yesct commented
Status: Needs work » Closed (duplicate)
Issue tags: -Novice, -Needs reroll

Closing this since the other issue #2030633: Expand FieldStorageConfig with methods will have to remove direct accesses of the properties since it will be making them protected. (this is a change from how we were approaching the issues originally.)