Part of meta-issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
See the detailed explanations there and look at the issues that already have patches or were committed.
Add get*, set* and additional methods as it makes sense to replace the public properties (e.g. isSomething() and something())
For beta evaluation, see the meta.
Proposed resolution
New methods added to the FieldConfigInterface. setTranslatable() was also added to FieldConfigInterface even though it was not introduced in this patch.
Here is a table with class properties and associated methods and notes about each to help reviewers. The elements in bold are the ones that have been added in this series of patches. The notes try to compile most of the previous comments into why we have or haven't added certain getters and setters.
Property | Visibility (before) | Visibility (after) | Get Method | Set Method | Notes |
---|---|---|---|---|---|
$id | public | protected (todo) | id() | N/A | The getter will eventually need to be changed to getId() per #2246695: replace all core usages of id() with getid(). There is no setter for $id because it is derived from $field_name and $entity_type |
$field_name | public | protected (todo) | getName() | N/A | There is no setter for $name because it is set in the constructor. See #26. |
$entity_type | public | protected (todo) | getTargetEntityTypeId() | N/A | There is no setter because it is set in the constructor. See #50. |
$type | public | protected (todo) | getType() | N/A | There is no setter for $name because it is set in the constructor. See #26. |
$module | public | protected (todo) | getTypeProvider() (see #154) | N/A | $module is derived from the field type. See #26. |
$settings | public | protected (todo) | getSetings() getSetting() |
setSettings() setSetting() |
setSettings will overwrite the entire $settings property. setSetting will set a key within the settings array. This functionality mimics the existing getSettings and getSetting methods. |
$cardinality | public | protected (todo) | getCardinality() | setCardinality() | |
$translatable | public | protected (todo) | isTranslatable() | setTranslatable() | Not added in this patch. setTranslatable was added to FieldConfigInterface. |
$locked | public | protected (todo) | isLocked() | setLocked() | |
$indexes | public | protected (todo) | N/A | N/A | There is no setter because it is set as part of the schema. See #50. |
$deleted | public | protected (done in #93) |
isDeleted() | N/A | |
$schema | protected | no change | getSchema() | N/A | Not added in this patch. |
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch making one of the properties protected (include an interdiff) | Novice | Instructions | done in #93 |
Create a patch replacing direct usages of the properties with calls to methods (test results from making the properties is useful) (include an interdiff) | Not novice | Instructions | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
For reviewers:
- Look at usages of methods and think about if we really need a method. (If all usages are in tests, the tests might be able to be better written to not require the method.)
- When reviewing, make sure all methods have visibility settings https://www.drupal.org/node/608152#visibility and doc blocks https://www.drupal.org/node/1354 Docs is a core gate. https://www.drupal.org/core-gates#documentation
Comment | File | Size | Author |
---|---|---|---|
#209 | interdiff.txt | 1.27 KB | Mile23 |
#209 | 2030633_209.patch | 85.18 KB | Mile23 |
Comments
Comment #1
japicoder CreditAttribution: japicoder commentedStart to work on this ...
Comment #2
YesCT CreditAttribution: YesCT commented@japicoder Did you have any questions or intermediate work to post?
Comment #3
swentel CreditAttribution: swentel commentedPostponed on #1953408: Remove ArrayAccess BC layer from field config entities
Comment #4
swentel CreditAttribution: swentel commentedpostponed now on #2095303: Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config'
Comment #5
Alumei CreditAttribution: Alumei commentedThat issue seems to be resolved. Back to active.
Comment #6
Alumei CreditAttribution: Alumei commentedList of public property's found via the FieldConfig API documentation:
Comment #7
Alumei CreditAttribution: Alumei commentedRenamed to match the new classname.
Comment #8
czigor CreditAttribution: czigor commentedComment #9
czigor CreditAttribution: czigor commentedThis is the first draft, changing only things in FieldConfig.php. As a consequence, properties are still public to see if the tests fail already. (See #14.)
Getters-setters table
('+' means added in the patch, '-' means none):
property; getter; setter
id; id(); -
name; getName(); +setName()
entity_type; getTargetEntityTypeId(); +setTargetEntityTypeId()
type; getType(); +setType()
module; +getModule(); +setModule()
settings; getSettings(); +setSettings() (also added a setSetting() method)
cardinality; getCardinality(); +setCardinality()
translatable; isTranslatable(); setTranslatable()
locked; isLocked(); +setLocked()
indexes; +getIndexes(); +setIndexes()
deleted; +isDeleted(); +setDeleted()
Not sure about
Comment #10
czigor CreditAttribution: czigor commentedComment #12
czigor CreditAttribution: czigor commentedComment #13
czigor CreditAttribution: czigor commentedPatch passed tests, so let's replace other occurences of direct property access to getters and setters.
Comment #14
czigor CreditAttribution: czigor commentedI found contradictory informations on whether the properties should be public or protected. The following patch sets all properties to protected. Also replaced all direct access to FieldConfig properties with getters and setters (ie. all that I found).
Comment #15
czigor CreditAttribution: czigor commentedComment #17
swentel CreditAttribution: swentel commentedNote - berdir told not to make them protected so the patch in #12 is good.
Comment #18
czigor CreditAttribution: czigor commentedSo the patch in #12 needs review.
Comment #19
msonnabaum CreditAttribution: msonnabaum commentedI'm sorry, but keeping the properties public completely defeats the purpose of this. The test failures on the protected version are showing very clearly that the job simply isn't finished.
Please make them protected and continue converting usages to use the new methods.
Comment #20
mashermike CreditAttribution: mashermike commentedNeeds a reroll of the patch from #14.
Comment #21
sundersingh CreditAttribution: sundersingh commentedWill be working on this task; assigned via core mentoring.
Comment #22
sundersingh CreditAttribution: sundersingh commentedComment #23
carsonevans CreditAttribution: carsonevans commentedI'm gonna attempt to tackle this.
Comment #24
carsonevans CreditAttribution: carsonevans commentedMy first patch. Hopefully it's not too ugly!
Comment #25
carsonevans CreditAttribution: carsonevans commentedI should have included this info in comment 24 but forgot to. Here is a summary of my tasks with a few notes about things I did not do and why...
Changes:
- added setName to set $name
- added setType to set $type
- added setModule and getModule to set/get $module
- added setCardinality to set $cardinality
- added lock() and unlock() to handle $locked
- added setSetting to set an individual setting by key
- added setSettings to overwrite the entire settings value
- added setIndexes and getIndexes to set/get $indexes
Did Not:
- Did not create a setter for $id because it is made up of entity_type and name
- Did not create a setting for $entity_type because it is set in the constructor
- Did not do anything with $deleted because the documentation mention it is set by a delete() function but I don't see where that function is?
- Did not change all references to $this->name to use $this->getName() or $this->type to $this->getType. Since there is no additional processing happening this seems redundant to me.
Comment #26
yched CreditAttribution: yched commented- setName(), setType() : those informations are required in the constructor. Do we really want to add setters for those ?
- setModule() : this is derived from the field type, so not sure there should be a setter either ?
Comment #27
carsonevans CreditAttribution: carsonevans commentedWorking with Bart @ DrupalCon Austin Sprint. We talked through yched's comments and agree so I've updated the patch to reflect them.
I removed:
- setName()
- setType()
- setModule()
Comment #28
markie CreditAttribution: markie commentedI have installed this patch successfully.
Comment #29
carsonevans CreditAttribution: carsonevans commentedTalking at the drupalcon sprint with WebChick and found a few issues that need to be addressed. I am going to go back and re-roll the patch from #12 and then implement yched's comments into that. Also need to add some documentation to a few things.
Comment #30
carsonevans CreditAttribution: carsonevans commentedThis is just a re-roll of czigor's patch from comment #12.
I still need to apply yched's comments but wanted to get the re-roll submitted before tackling that. I'll continue to work on that if this passes the tests.
Comment #31
carsonevans CreditAttribution: carsonevans commentedForgot to mark as needs review to queue the test.
Comment #32
carsonevans CreditAttribution: carsonevans commentedRemove setName(), setType(), and setModule()
Comment #34
carsonevans CreditAttribution: carsonevans commentedHad a reference to the deleted setModule(). I fixed that.
Comment #35
carsonevans CreditAttribution: carsonevans commentedComment #36
HaloFX CreditAttribution: HaloFX commentedApplied patch from #34. Applied without error, nothing blew up.
Enabled, disabled, and re enabled all field related modules.
Comment #37
HaloFX CreditAttribution: HaloFX commentedKicking back to needs review for a more in depth look.
Comment #38
Geijutsuka CreditAttribution: Geijutsuka commentedAdded type hinting where necessary and added comment to locked flag. Big thanks to YesCT for throwing this our way (my first patch!).
Shoutout to:
@AndyThornton
@jackbravo
Comment #39
carsonevans CreditAttribution: carsonevans commentedThis patch intentionally goes against the current standard of making properties public for testing purposes.
This patch will get tested by the testing system and test the assumptions about getters and setters in the rest of the code are correct and to identify any places in core that might need to be updated to use the new getters/setters.
I will submit a corrective patch right after it gets tested.
Comment #41
carsonevans CreditAttribution: carsonevans commentedI need some advice on next steps here. There are definitely places where FieldConfig properties are referenced. The failed test points to line 1351 of ContentEntityDatabaseStorage which references deleted instead of using isDeleted(). But the test stops there after it has failed. Three questions I have...
A) For this patch (#38) to be accepted do all the places that reference it as a public property also need to be updated?
B) If so, I have not used SimpleTest before but I assume there is a way for me to get all failures not just the first failure? Otherwise uploading a patch to fix one issue just to wait for test bot is going to be painful. I guess I need to learn simpletest and SimplyTest.me to move this forward.
C) Also, the method in ContentEntityDatabaseStorage type hints a FieldConfigInterface object for the $field it works with. FieldConfigInterface is fairly sparse, only defining isLocked and getBundles. Should we augment it to have a isDeleted(), and potentially other functions, as well?
Comment #42
yched CreditAttribution: yched commentedYeah, I'd tend to think this patch doesn't have to convert all direct accesses to the public properties
Comment #43
carsonevans CreditAttribution: carsonevans commentedI have uploaded patch from 38 renamed to match this comment number as it was the last passing (and should still pass) patch.
I am not uploading an interdiff because it is the negation of the last interdiff, if that makes sense. If that's incorrect procedure let me know.
I am setting this to needs review to kick off test bot and move it along.
If we want to create a separate issue to change the direct-access to use setters who would do that? I'd like to take a stab at it if that's ok?
Comment #44
yched CreditAttribution: yched commented@carsonevans
Sure, go for it :-) You can create the issue yourself (tag it with "Novice, Entity Field API"), and link to it in a comment here ?
Comment #45
carsonevans CreditAttribution: carsonevans commentedI created the issue at #2282299: Accessing FieldConfig properties should be done through methods.
Comment #46
carsonevans CreditAttribution: carsonevans commentedThe patch needed to be re-rolled because of format_string was changed to String::format on lines 301, 311, 317, 467
I re-rolled it but couldn't figure out how to do an interdiff that just had the changes that were conflicting.
Comment #47
carsonevans CreditAttribution: carsonevans commentedComment #48
YesCT CreditAttribution: YesCT commentedI read through all the lines in this patch. It looks ok to me.
I have not looked at the class to see if any properties are missing getters and setters.
Comment #49
carsonevans CreditAttribution: carsonevans commentedHere is a table with class properties and associated methods and notes about each to help reviewers. The elements in bold are the ones that have been added in this series of patches.
EDIT: Deleted Table. Reproduced in #51 below with more recent data.
Comment #50
BerdirThere is no need for setTargetEntityTypeId() because that is also set in the constructor and can not be changed.
Instead of setLocked(), it would also be possible to have lock() and unlock(), not sure what's more clear.
Not sure about getIndexes().. if you look at the documentation, it's never accessed directly from the outside but always only merged into what is returned by getSchema(). I suggest you removed getIndexes() as it could be misleading (does it only return custom indexes or also those defined by the field type?)
New methods should be documented on the interface (FieldConfigInterface) and the implementation should only refer to it with {@inheritdoc}
Comment #51
carsonevans CreditAttribution: carsonevans commentedAll new methods have now been added to the FieldConfigInterface. setTranslatable() was also added to FieldConfigInterface even though it was not introduced in this patch.
Here is a table with class properties and associated methods and notes about each to help reviewers. The elements in bold are the ones that have been added in this series of patches. The notes try to compile most of the previous comments into why we have or haven't added certain getters and setters.
There is no setter for $id because it is dervied from $name and $entity_type
getSetting()
setSetting()
Comment #53
carsonevans CreditAttribution: carsonevans commentedWhoops. Left a getIndexes() in after removing it per #50.
Interdiff-51-53.txt will show the specific error fixed in this latest patch.
Interdiff 46-53.txt is the full meat of the interface and documentation updates that incorporate bedir's comments.
Comment #54
carsonevans CreditAttribution: carsonevans commentedComment #56
carsonevans CreditAttribution: carsonevans commented53: 2030633-expand-fieldconfig-and-interface-with-methods-53.patch queued for re-testing.
Comment #57
carsonevans CreditAttribution: carsonevans commentedLanguageFallbackTest.php failed the first time through. I scanned it but didn't see how it affected FieldConfig so I just re-queued the test. Second time through it passed. I'm not sure if that should be worrisome or not.
Comment #58
BerdirLooks good to me. Remember to set the issue back to needs review.
Comment #60
amitgoyal CreditAttribution: amitgoyal commentedPatch in #53 no longer applies. Please review updated patch which applies cleanly.
Interdiff failed to create interdiff.txt file so I am not attaching it.
Comment #62
BerdirFieldConfig is now FieldStorageConfig.
Comment #64
undertext CreditAttribution: undertext commentedComment #65
m1r1k CreditAttribution: m1r1k commentedComment #68
penyaskitoRetitling.
Comment #69
Jānis Bebrītis CreditAttribution: Jānis Bebrītis commentedRe-rolling #64 for FieldStorageConfig and latest checkout.
Comment #70
daffie CreditAttribution: daffie commentedThe entity variables are not protected. They must be. The idea of encapsulation is that you cannot access the variables directly. You must use the functions getVariableName() and setVariableName(). If the variable is a boolean you have the function isVariableName().
The patch is almost a month old so also a reroll.
Comment #71
yched CreditAttribution: yched commentedIs it what was done in the other issues in #2016679: Expand Entity Type interfaces to provide methods, protect the properties ? Most properties of ConfigEntities are still public, aren't they ?
Comment #72
daffie CreditAttribution: daffie commented@yched: As far as I know is that the variables must be protected. The whole idea for doing is so is that we have encapsulation of the entity variables. Please correct me if I am wrong.
This is what alexpott said in #2030645: Expand Menu with methods comment #31:
Comment #73
yched CreditAttribution: yched commented@daffie: OK, right, this is the new direction for #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
Never mind my #71, #70 is correct.
Comment #76
daffie CreditAttribution: daffie commentedThe most important problem is that the class variables are not protected. The whole idea of adding functions to get and set the class variables is the principle of encapsulation.
A minor issue:
You need to change @return $this to
Comment #77
Mile23@return $this is just fine: https://www.drupal.org/coding-standards/docs#types
Here's a re-roll of #69.
Comment #78
Mile23Comment #80
Mile23Let's fix that re-roll... :-)
Comment #81
daffie CreditAttribution: daffie commented@Mile23: The class variables are still public. They must be protected.
Comment #82
daffie CreditAttribution: daffie commentedComment #83
YesCT CreditAttribution: YesCT commentedneeds tests, and I think one of the methods is missing a doc block.
We should also think about wether we actually need all these methods, so a reviewer (or someone) should see if/where (in tests?) the methods are used.
this will be an api change.
making the properties protected allows us to make sure we replace all the usages of direct access to them.
And else where we decided that public properties were a bug... so I think we should have the properties protected in this issue also (not sure if the meta is up to date will all this policy/process changes). (See the issue summary in the meta)
OH. this had a follow up to replace all the direct accesses...
#2282299: Accessing FieldConfig properties should be done through methods
Which means... we would have to leave the properties public.
What a mess.
Need to see how a recent one of these issues went in.
Comment #84
YesCT CreditAttribution: YesCT commentedmoving this to bug similar to #2030645-31: Expand Menu with methods
and closing #2282299: Accessing FieldConfig properties should be done through methods since we will make the properties protected here.
also updating the issue summary.
https://www.drupal.org/core-mentoring/novice-tasks
The whole issue is not novice, but the next step can be:
making the properties protected.
Make a new patch and add an interdiff. Set to needs review to trigger the test bot. (We know there is more to do here than just that, but it's ok to make little steps of progress.)
Comment #85
daffie CreditAttribution: daffie commented@YesCT: #2030661: Expand Tour with methods landed 4 days ago and #2030613: Expand EntityViewMode (really EntityDisplayModeBase) with methods landed 5 days ago. Alexpott wants this fixed because he thinks that the exposure of properties as public allows code to not use the API eg ->id vs ->id(). "This makes Drupal more stable and prevents bugs." (see comment #72)
And yes we can add tests, but is that really necessary for getters and setters?
Comment #86
YesCT CreditAttribution: YesCT commentedOh right, if they are just getters and setters, then tests might not be needed. (I googled, and the internet is full of opinions on if getters and setters need to be unit tested.) Maybe just keep your eye out for any methods that are more complicated that might need tests, and update issue summaries to point them out.
updating the summary to take adding tests out of the remaining tasks.
Comment #87
Mile23I attacked this problem a bit yesterday and it's a huge refactoring task. It's not like FieldStorageConfig is used by *everything.* :-)
There's no way it's a novice task, and it will be heck to review.
Also, it's not a needs-tests issue, because the tests tell you whether you're on the right path or not. Do not add tests at this point; they'll only be confusing.
I'd suggest going through and changing one property to protected, then make all the tests pass. Then change the next property to protected and make all the tests pass, and so forth. There's no way to parcel this out as multiple issues because of the way some code deals with these properties... Patches would overlap in irreconcilable ways. It might be possible for participants to change one or two properties at a time and then post a patch with an interdiff. Then the next person can work on another property or two after the testbot passes the previous patch.
The relevant tests during this refactoring will be the ones related to field module. Then of course run all of them and see if you're really done. :-)
Ideally, someone who thought these properties should be public would be the one to go through and do this. [evil grin]
Comment #88
daffie CreditAttribution: daffie commentedBefore we start making the patch. I would like us to agree on a few points:
Maybe a stupid question but can this issue be do with one patch file or do we need multiple patch files?
If alexpott is the one who is going to commit this patch, I would also like a confirmation from him about this.
Comment #89
Mile23Comment #90
YesCT CreditAttribution: YesCT commentedopps. you are totally right, I meant to only make the task to make them protected a novice task.
changing all the usages is not novice.
updating the issue summary.
Comment #91
YesCT CreditAttribution: YesCT commentedComment #92
daffie CreditAttribution: daffie commented@Mile23: I know that there are in your patch. What I really would like is that alexpott signs off on that. So that when the patch is ready and RTBC, we do not get in the discussion over which getter and setter function should or should not be in the patch. With the result that that will take some time and then we need to reroll the patch.
Comment #93
fernando_calsa CreditAttribution: fernando_calsa commentedThis is my first collaboration to an issue, I have changed the $deleted to protected.
Comment #94
YesCT CreditAttribution: YesCT commentedThank you @fernando_calsa
Setting to needs review to trigger the testbot. (d.o was making @fernando_calsa wait, thinking the new account might be spamming the comments. I gave them the not the spammer role, which should help for the future.)
@daffie I dont think we should have to clear it with a committer right now, Berdir or yched should be fine (or other person from Maintainers.txt for field stuff).
I think we should read the previous comments from Berdir and yched (if there are any) in this issue about the naming of methods.
And we should put the table in #51 in the issue summary.
I can also look more in depth at that and see what I think about the names and methods, but I would have to read all the comments on the issue and think about it... so let's at least do that before we ping a committer (or Berdir or yched).
Comment #95
YesCT CreditAttribution: YesCT commentedJust a start at summarizing what methods should or shouldn't be in this patch, by copying the table from #51 into the summary.
Comment #96
daffie CreditAttribution: daffie commentedComment #98
YesCT CreditAttribution: YesCT commentedupdated the summary remaining tasks to indicate the novice part is done (for now).
also added a column to note change to the property visibility that this patch does.
Comment #99
YesCT CreditAttribution: YesCT commentedoops. just a copy paste mistake.
Comment #100
daffie CreditAttribution: daffie commentedIn the issue summery we added the table from comment #51. The table is a proposal for which getter and setter functions to add and not to add to the FieldStorageConfig class. Can one of the field system component maintainers or a branch maintainer sign of on this. This is going to be a big patch and a lot work. In some other sub-issues of #2016679: Expand Entity Type interfaces to provide methods, protect the properties we are getting in a discussion when the patch is made and RTBC over which getter and setter functions to include. The problem for this issue is that that will result in a reroll and a lot of extra work. So I really want an agreement over which getter and setter functions will be included.
Comment #101
daffie CreditAttribution: daffie commentedComment #102
daffie CreditAttribution: daffie commentedTalked with alexpott on IRC and think that most functions are oke. But he want to remove setDeleted() and setIndexes(). I have updated the issue summary.
Comment #103
Mile23Note that some settings stuff might be solved over here: #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface
Comment #104
Mile23Comment #105
Mile23This patch makes all properties of FieldStorgeConfig protected except for:
entity_type
,field_name
,settings
, andtype
.It removes
setDeleted()
andsetIndexes()
from previous patches (and rightly so, I'd wager).It passes the field module's tests, to the lazy extent that I worked on that, but let's see what the testbot says.
Update: aw crap, I named the interdiff with .patch. Sigh.
Comment #110
Mile23Found a lot more. This might fail testing, but it shouldn't have fatal errors.
Comment #112
Mile23Fixing a few things... More later as time permits.
Did you know that
Drupal\field_ui\Tests\ManageFieldsTest
takes 15 minutes to run locally?Comment #114
Mile23This should fix all the test failures since #105.
This patch makes all properties of
FieldStorgeConfig
protected except for:entity_type
,field_name
,settings
, andtype
.I'll get those encapuslated later.
Comment #115
Mile23Add
$settings
to the list of protected properties.Comment #117
Mile23And now, a blocker: #2398901: Image module makes fatal assumptions about EntityInterface
Comment #118
YesCT CreditAttribution: YesCT commentedThank you @Mile23 for moving these issues forward. You have done a lot on these issues.
in
#2398901-16: Image module makes fatal assumptions about EntityInterface
@yched suggests 3 ways forward.
I'm in favor of:
--
Unassigning so it is clear anyone can try to do that.
@Mile23 if you start to try that, just make a comment here, so we can avoid two people both doing it.
See also #2030637-39: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods
Comment #119
BerdirOk, here is a patch that fixes image_entity_presave(), without needing anything more than we already have here.
Notes:
- This does not contradict what I said in [#9466051], I switched to interface checks as that gives better IDE integration, but it would work just as well with checking the entity type ID.
- This method works with exactly two entity types/objects/interfaces. field configs and field storage configs. I'm making it a bit more explicit, but not changing any actual logic, it already did exactly this before.
- There is a fair amount of old code that has only been partially ported. For example, loading the field storage is not necessary, we already have getType() on both interfaces for this. And the default settings are also applied automatically, so we can drop that.
- We can just drop the if/else at the end in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods, once both interfaces have a setSetting() method.
- This is IMHO better than having a common interface, because a generic settings interface would be meaningless. We know that those two entity types have settings and that we can change them. A generic (entity or data definition) interface with setters for settings does not tell you anything about what kind of settings it includes. That's just an object-version of an array. Also note that FieldStorageConfig does implement DataDefinitionInterface, #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface would us help nothing here.
If you have questions, ping me in IRC, I think that is a better place to discuss this through than the issue queue.
Comment #120
Mile23Disagree, but thanks. :-)
I can live with the approach of #119, assuming tests pass.
Still to go on this issue: Encapsulating
$entity_type
,$field_name
, and$type
.Comment #122
hussainwebFixing failures.
Comment #124
hussainwebThat line of code would have worked in PHP 5.5. That is why the IDE didn't report it. Fixing it for PHP 5.4.
Comment #126
hussainwebFixing more failures.
Comment #128
hussainwebFixing more failures. The exception is fixed. Let's see if we find any more places accessing the property directly.
Comment #130
hussainwebFixing the typo and another use of settings property. I think that some of the failures might be random. Let's see if they come up again.
Comment #132
hussainwebFixing the problem before moving on. I am sorry for so many interdiffs and patches. :)
Comment #133
hussainwebMoving on with
$type
property. I am beginning to think it would be better with these as separate issues. The interdiff alone is 15 KB and I am replacing only those that I found with a simple regex. I am sure tests will tell us more.Comment #135
hussainwebIt seems there was only one cause for all the errors. Let's see.
Comment #136
hussainwebMoving on to
$entity_type
property. I am replacing it withgetEntityTypeId()
method wherever I found it. On to the testbot.Comment #138
BerdirYou need to use getTargetEntityTypeId(), get EntityTypeId() is the entity type of your own entity (field_storage_config)
Comment #139
hussainweb@Berdir: I actually meant getTargetEntityTypeId() in my previous comment (#136). But it looks like I have used getEntityTypeId() in some places. Fixing that now.
Comment #140
hussainwebFixing the method calls as per #138.
Comment #141
hussainwebThe last property now:
$field_name
. I am replacing it withgetName()
method wherever I found it. May the testbots smile upon it!Comment #142
hussainwebIt's been 5 hours and it still shows "Test request sent". I know this is not normal but does this mean that we should initiate a retest somehow?
Comment #145
hussainwebFixing the failure.
Comment #146
hussainwebYay! All properties are now protected and all tests are passing. I don't think there is anything else left to do except review.
Comment #147
daffie CreditAttribution: daffie commentedGood work hussainweb!
Why is this code necessary for this issue?
Why not do "if ($this->isDeleted()) {"
Good find!!!
I think that directly using $field_storage->getName() is better.
Nitpick: add a space between $key, and $value.
This is not solvable. There is problem with the function image_entity_presave(). See comment https://www.drupal.org/node/2398901#comment-9466051. I think the best option is to merge #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods with this issue. Which I did.
Comment #148
BerdirIt is solved or at least will only require a minor update in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods, which we should postpone on this. Merging the patches will probably double the patch size of this issue again.
Comment #149
daffie CreditAttribution: daffie commentedComment #150
daffie CreditAttribution: daffie commentedPlease use the solution from #2398901: Image module makes fatal assumptions about EntityInterface:
In #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods shall we remove this construction.
Comment #151
BerdirThe patch is using interfaces for that, which is the same.
Comment #152
daffie CreditAttribution: daffie commentedTo explain the change from comment #150 a little bit more. Change in file image.module on line 343 in the function image_entity_presave() to:
In #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods shall we remove this construction. After that can we easily commit the interface from #2398901: Image module makes fatal assumptions about EntityInterface.
Comment #153
BerdirAgain, image_entity_presave() is already perfectly fine as far as this issue goes, both objects have a getSetting(), only setSetting() is missing on field_config, and that will be trivial to update in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods. And I still insist that #2398901: Image module makes fatal assumptions about EntityInterface is going in a wrong direction and is a won't fix.
Confused why this is still module and not provider. Also fun fact: It is "core" for field types defined in Drupal\Core\Field :)
But that has nothing to do with this issue, *except* that we might want to name the method getProvider() instead. Not sure.
As @daffie mentioned, this is strange. I would like to see what exactly fails without adding this, can we have a patch without those changes and then see what happens?
Same here, no need to change anything here, so drop this.
yes, as mentioned above, if (!$this->isDeleted()) should be enough here.
Why are some methods methods using $this->set() and others not?
This should be unified. I'm not sure if it as a good idea, but #2399965: Config entity get() and set() should invoke getters/setters wants to change set() so that it calls e.g. setModule(), so calling set() would result in a loop and we should probably access the property directly.
I don't think this should talk about a property, just write something like "Returns TRUE if the field was deleted, FALSE otherwise" or so.
"array" is an implementation detail, I'd write something like "Sets field settings (overwrites existing settings)."
string is wrong.
As mentioned above, we use almost everywhere "provider" instead of module now, so maybe we should use that for the method name.
This could also use $field->get($key), I'm not sure if this list is really complete. Because the default will definitely not work anymore.
No, we do not, set('type', 'integer') works too.
We usually do not prefix functions with a \.
Same, use set() Or add a setter, because this seems like a valid thing that you might want to set?
Comment #154
yched CreditAttribution: yched commentedre: #153.1 :
$storage->module / $storage->getModule() :
+1. This issue is breaking BC anyway, we might as well move to something meaningful. Could even be getFieldTypeProvider(), for clarity. This is not the provider of the "storage definition".
re: #153.5 :
Noticed the same pattern in #2030607: Expand EntityDisplayBase with methods, was wondering about that too.
Comment #155
Berdir@yched: Thanks for the +1 ;) Do you know a reason why we couldn't get the provider from the plugin definition directly instead of persisting it? I understand that was convenient to have earlier, but when it is a method, it shouldn't matter? that's not a change that belongs in this issue of course, just wondering.
Comment #156
Mile23Some of these issues use
set()
and some don't because it's completely unclear when one should or shouldn't useset()
.set()
has side-effects. Are they necessary? Is it an API or just something that's there for some reason?Comment #157
yched CreditAttribution: yched commented@Berdir #155:
Good question.
IIRC, we persisted it in the field definition to be able to know it in case the providing module went missing (disabled, etc)...
I think that comes from CCK, and was kept in the D7 Field API port.
Later in the D7 cycle, "prevent disabling modules that provide field types in use" was added, and in D8 that area moved further with config dependencies and stuff. So it might be that we could stop saving it in the config files now, yes.
Not directly related to the current patch, though, so it's probably best as a followup / separate issue - opened #2407463: Stop storing the provider module in FieldStorageConfig yamls.
As for this issue here : the current uses of $field_storage->getModule() evidenced by the current patch show that it's still handy to have a method to access it from the $field_storage, so the said followup would still keep the method.
Thus, still +1 on giving it a clearer name like getFieldTypeProvider() in this patch here.
Comment #158
hussainwebLot of reviews to work on! I am starting with a reroll. It was quite a massive reroll considering that it is just 15 days. There were conflicts to setting translateable on fields, better loading dump files in migrate tests, #2357801: File field default values are not deployable -- use UUIDs for the default file and some smaller ones.
Setting it to needs review to let this run through the testbot. I will set it to needs work once tests run and will then work on the reviews. I am also adding "Needs issue summary update" as it seems that some of the comments above will need it.
Comment #160
hussainwebI was afraid I'd miss something. Trying again.
Comment #162
hussainwebSorry, it was the exact same patch. Gotta focus.
Comment #164
hussainwebFixing the error (and in other places). I am hoping there won't be many like these as it is not long since the last pass.
Comment #166
hussainwebComment #167
hussainwebLooks good. Setting to needs work for review points.
Comment #168
hussainwebWorking on the simpler review points first.
From #147,
1. Moving this to the next patch.
4. Is there a specific reason why calling
getName()
every time would be better? I know it does not make a big performance difference but on the other hand, it does not make the code harder to read. Do you still think I should change it?From #153,
1. Do you think I should rename the property as well? If I understand this correctly, I don't think we can rename the properties without affecting the YAML files as well. In that case, the getter/setter name would be different from the property. I don't think it matters considering we are going to #2407463: Stop storing the provider module in FieldStorageConfig yamls.
2. I will try a separate patch for this.
5. I changed everything I could find to set the property directly.
9. Same as 1.
13. Added getter and setter for
indexes
property.Setting it to needs review for testing.
Comment #169
BerdirYes, keep ->module, don't rename that. It is fine if they don't have the same name.
Comment #170
hussainwebChanging
getModule()
togetFieldTypeProvider()
as per #154. I am also updating the IS to update the getter name.Comment #171
hussainwebI am creating a patch to see what goes wrong after removing the code mentioned in #147.1 and #153.2.
Comment #172
hussainwebSo, I guess you don't need that line after all.
I think all the tasks are done. Am I missing something? Please review.
Comment #173
daffie CreditAttribution: daffie commentedThese two methods are not necessary. The method getIndexes() is not used anywhere and setIndexes() is only used in tests. Also there is in the issue summary a list with methods to be added. These are not on that list.
I think that it is better to replace $field_storage->field_name directly with $field_storage->getName().
Nitpick: Can we be consistent and use $field->getSetting('default_image') instead of $field->getSettings()['default_image']
You are changing here far to much. Leave the 'field_config' stuff alone. My suggestion is to change as little as possible.
Can we change FieldConfig to FieldConfigInterface and remove the line with "use Drupal\field\Entity\FieldConfig".
Comment #174
Berdir1. It is an API. It was added because I suggested to do so above. If missing tests is a blocker then I suggest we simply add a few lines to get the indexes again where we are setting them to verify that part works.
4. No, that is not changing too much. As I've tried to explain you 4 times in this issue already, this method is changing exactly as much as we need there, nothing less, nothing more.
5. I disagree. We access a property, not a method, the property is not defined on the interface. As said before, this will go away as soon as FieldConfig also gets a setSetting() method.
Comment #175
yched CreditAttribution: yched commentedI still intend to review this at some point very soon, sorry for not being able to do it earlier.
For now, just : agreed with @Berdir on all of #174
Comment #176
Mile23#171 needs reroll
Comment #177
Mile23#171 introduces
FieldStorageConfig::isRequired()
, which is marked up as{@inheritdoc}
, but I can't find the method it's overloading. I know this because it's pretty much the reason for the only merge conflict in the reroll. All it does is return FALSE. I've left it in for now.This doesn't address any changes suggested since #171.
Comment #178
Mile23Comment #179
Berdir=> I don't think this issue introduced it as it existed in HEAD, but it probably started appearing in the context. Remove it.
Comment #181
Mile23Thanks @daffie for the review.
#173.2: Much better to explicitly call
getName()
. Also, I changed the generic, reused$field_storage
to a number of descriptive names which make the test easier to read.#173.3: If you check the comment right above that line, it says this:
// The value of a managed_file element can be an array if #extended == TRUE.
I'm not sure whether that's actually true, but the image module's tests pass, so I'm making the change and taking out the comment. (Oops... made a patch with the comment in. Will change that on the next go-round.)#173.1,4, and 5: Not changing those as per @Berdir in #174.
Also removed
isRequired()
as per #179.The last patch failed the migration test, so let's see if that happens again.
Comment #183
Mile23I can't make migrate module pass tests locally against HEAD, so I'll just wait a day or something.
Comment #184
Mile23OK, retesting.
Comment #187
Mile23The failing test is MigrateDrupal6Test, which I still can't get to pass locally against HEAD.
Comment #190
Mile23Reroll.
Comment #192
Mile23Missed one.
Update: Ugh. Looks like I uploaded twice.
Comment #193
daffie CreditAttribution: daffie commentedIt all looks good to me.
All the methods from the issue summary are added.
Methods getIndexes() and setIndexes() are also added on request of @Berdir (see comment #174). I do not think that is necessary to add those two methods. The first method is never used and the second only in tests. But @Berdir is on the drupal 8 maintainers list, so I am following his lead.
All documentation is in order.
It get a RTBC from me.
Comment #194
yched CreditAttribution: yched commentedThanks a ton for sticking with this, folks.
Been kept in other issues, but I'd really like to have a last look at this before it gets in. Will try hard to do that tomorrow night.
Comment #195
daffie CreditAttribution: daffie commentedCan we change the to do issue to #2398901: Image module makes fatal assumptions about EntityInterface. The current to do issue #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods does not fix the problem.
Comment #196
BerdirNo, we can not. Either this or the other issue needs to get in first, and then the other one needs to be updated.
#2398901: Image module makes fatal assumptions about EntityInterface is still a won't fix in my book.
Comment #197
yched CreditAttribution: yched commentedSame here :-) I stick with what I said in #2398901-14: Image module makes fatal assumptions about EntityInterface.
Comment #198
Mile23#2398901: Image module makes fatal assumptions about EntityInterface is, as noted, a Bug From The Future, and since it's not the future yet, it stands to reason that we won't address it in the present. Let's just get through what we can get through now.
Any review appreciated.
Comment #199
yched CreditAttribution: yched commentedThanks for the great work folks ! (and thanks for bearing with my slow reviewing :-/)
This is mostly ready IMO, just a few nitpicks below:
Not related, and I'm not a fan of the var renames :-)
Reusing the same simple var name through the course of a test is frequent - can we set them back to $field_storage ?
We should polish the method order so that related methods are next to each other.
(and same in the implementation class)
Proposed order :
Which also shows that getFieldTypeProvider() should actually be getTypeProvider() (sorry, IIRC I'm the one who suggested that name :-/). We don't use the "Field" prefix anywhere else in that interface.
I get it for 'module', but why can't 'deleted', 'type', 'field_name' go through the default case ?
Extra empty line at the beginning of the function
*Please* let's not rehash the discussion in #2398901: Image module makes fatal assumptions about EntityInterface :-)
Doing stuff based on $entity->getEntityTypeId() in hook_entity_Xxx() is what Core does. Let's not change it here.
should type hint as array
Same here
Comment #200
Berdir5. Ha, I changed it to use interfaces :)
IMHO, it doesn't matter which method you use to identify the entity type. Implementing a type specific hook actually uses *both* methods at the same time, so it can be argued that either is fine :) Using the interface means that you get the right type hints and visual confirmation that those methods really exist and without that, going through the effort to add methods and use them is only half as much fun. The difference to #2398901: Image module makes fatal assumptions about EntityInterface is that we use existing, real interfaces that have a meaning.
If you look at existing implementations in core, you'll notice that interface-based checks are actually quite common, although they are usually limited to checking if it is a config or content entity type. Specific entity type checks in the generic hooks are pretty rare, because then you usually use the type specific one. But as mentioned, all of those actually include a type specific interface check too ;)
Comment #201
yched CreditAttribution: yched commentedOh, I see. Yeah, this works for me. Sorry for the noise, forget #199.5
Comment #202
Mile23Thanks for the review. I'll poke at it later this evening.
Because those properties are now encapsulated and protected, which is the whole point of this issue. :-) There might be a better way to map properties to getters... Suggestions welcome. We *could* just grab them using reflection or something, but part of encapsulation is that there might be side-effects to the getter.
Comment #203
Mile23Oooookay.
Addressing everything in #199, except .3 and .5.
For .2 I realize now I only changed the order in the interface and not the class, but I'll wait for green from this patch before I fix it.
Comment #205
Mile23:-(
Comment #207
Mile23Comment #208
yched CreditAttribution: yched commented#199.3
Sure, but the default case does $field->get($key), so this is not a question of public/protected properties :-)
Actually, even 'module' could go through the default case - when I wrote the above I was mid-review and somehow thought we had renamed the internal 'module' property, but that was wrong.
So, most those cases could just go through the default ?
Comment #209
Mile23I tried it before and it blew up. Tried it just now and it didn't. Let's see what the testbot thinks.
Comment #210
daffie CreditAttribution: daffie commentedIt all looks good to me.
All the methods from the issue summary are added.
Methods getIndexes() and setIndexes() are also added on request of @Berdir (see comment #174). I do not think that is necessary to add those two methods. The first method is never used and the second only in tests. But @Berdir is on the drupal 8 maintainers list, so I am following his lead.
All points from @yched from #199 are addressed.
All documentation is in order.
It get a RTBC from me.
Comment #211
yched CreditAttribution: yched commentedyup, RTBC +1
Thanks a lot folks, see you in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods - will likely need a reroll when this one gets in ?
Comment #212
alexpottI'm not a huge fan of adding unused and untested methods (setLocked(), setSettings(), getIndexes()) and API methods that are only used in tests (setCardinality(), setTranslatable() and setIndexes()). But @Berdir and @yched have okay this and I'm not going to block the patch on that.
Committed eebf624 and pushed to 8.0.x. Thanks!
Beta evaluation is in the meta issue.