Problem/Motivation

Following plublic methods have are not defined on any interface:

DefaultTableMapping::setFieldNames
DefaultTableMapping::setExtraColumns
DefaultTableMapping::allowsSharedTableStorage
DefaultTableMapping::requiresDedicatedTableStorage
DefaultTableMapping::getDedicatedTableNames
DefaultTableMapping::getDedicatedDataTableName
DefaultTableMapping::getDedicatedRevisionTableName

Proposed resolution

Either add all the DefaultTableMapping public methods to TableMappingInterface or introduce new interface

Remaining tasks

Finalize the aporch and write a patch

User interface changes

None

API changes

None

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because simple adds method to interface
Issue priority Normal because nothing is broken
Disruption Non Disruptive for core/contributed and custom modules/themes
CommentFileSizeAuthor
#6 add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-6.patch7.27 KBysaid
#4 add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-4.patch7.36 KBysaid

Comments

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes
Parent issue: #2274017: Make SqlContentEntityStorage table mapping agnostic »
dawehner’s picture

dawehner
Primary language German
commented

No, interfaces are there because they are designed like that. Just adding all the methods is not always the best idea. I think its fine in some code to bypass the interface
and use just a specific instance, but ideally no code should have to rely on those public methods.

ysaid’s picture

ysaid commented
Assigned: Unassigned » ysaid
ysaid’s picture

ysaid commented
Status: Active » Needs review
StatusFileSize
new add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-4.patch7.36 KB

I created a patch and done the following :

- 7 public methods of DefaultTableMapping are now declared in TableMappingInterface

- Jibran mentioned DefaultTableMapping::generateFieldTableName in the issue description but it's a protected method. I Didn't include it in TableMappingInterface

It's my first patch and contribution to drupal core, please be kind :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes
Issue tags: +Quick fix

Thank you for your contribution @ysaid. Changes look good only minor spacing issue to address. I have also removed DefaultTableMapping::generateFieldTableName from issue summary.

+++ b/core/lib/Drupal/Core/Entity/Sql/TableMappingInterface.php
@@ -115,5 +115,86 @@ public function getFieldColumnName(FieldStorageDefinitionInterface $storage_defi
+    ¶
...
+    ¶
...
+  	
...
+     ¶
...
+  ¶
...
+  ¶
...
+  ¶

Extra white spacing.

ysaid’s picture

ysaid commented
StatusFileSize
new add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-6.patch7.27 KB

Thanks for your feedback @jibran. I fixed spaces issues. Please check it.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the fix @ysaid. It is easier for reviewers if you upload an interdiff.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Assigned: ysaid » plach
Status: Reviewed & tested by the community » Needs review

No so fast :)

See #2. Just because those methods are public doesn't mean they are supposed to be on an or that interface.

At least @plach should provide his opinion on this, there might be good reasons why this wasn't done.

dawehner’s picture

dawehner
Primary language German
commented

See #2. Just because those methods are public doesn't mean they are supposed to be on an or that interface.

So +1 to that statement. The interface is designed for a generic table mapping not specific to any implementation vs. the DefaultMapping which has the notion of those specific table types.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

What #9 said. Thanks for the patch @ysaid, it looks perfect, but this is a won't fix by me.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Assigned: plach » Unassigned

Or better: a "works as designed" :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Closed (works as designed)

As per #9 and #10.