Add all the DefaultTableMapping public methods to TableMappingInterface [#2542282]

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

Comment	File	Size	Author
#6	add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-6.patch	7.27 KB	ysaid
#6	8.0.x: PHP 5.5 & MySQL 5.5 13,168 pass
#4	add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-4.patch	7.36 KB	ysaid
#4	8.0.x: PHP 5.5 & MySQL 5.5 13,137 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 29 July 2015 at 17:39

Issue summary:	View changes
Parent issue:	#2274017: Make SqlContentEntityStorage table mapping agnostic	»

Comment #2

dawehner

German

CreditAttribution: dawehner as a volunteer commented 29 July 2015 at 17:40

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.

Comment #3

ysaid CreditAttribution: ysaid as a volunteer commented 2 August 2015 at 20:38

Assigned:

Unassigned

» ysaid

Comment #4

ysaid CreditAttribution: ysaid as a volunteer commented 2 August 2015 at 21:22

Status:

Active

» Needs review

File	Size
add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-4.patch	7.36 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,137 pass

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 :)

Comment #5

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 2 August 2015 at 21:51

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.

Comment #6

ysaid CreditAttribution: ysaid as a volunteer commented 6 August 2015 at 07:06

File	Size
add-DefaultTableMapping-public-methods-to-TableMappingInterface-2542282-6.patch	7.27 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,168 pass

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

Comment #7

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer commented 6 August 2015 at 10:57

Status:

Needs review

» Reviewed & tested by the community

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

Comment #8

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 6 August 2015 at 11:26

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.

Comment #9

dawehner

German

CreditAttribution: dawehner as a volunteer commented 10 August 2015 at 06:39

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.