Problem/Motivation

ContentEntityDatabaseStorage is a misleading name, since the only supported DBMSes are SQL-based ones. NoSQL storage implementations would need to extend ContentEntityStorageBase (or start from scratch).

Proposed resolution

Since we already have a SqlEntityStorageInterface, for consistency we should rename ContentEntityDatabaseStorage to SqlContentEntityStorage.

Remaining tasks

  • Agree on a solution
  • Code it
  • Review it

User interface changes

none

API changes

Renamed ContentEntityDatabaseStorage to SqlContentEntityStorage.

Files: 
CommentFileSizeAuthor
#20 entity-rename_cedb-2330091-20.patch45.47 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,868 pass(es). View
#20 entity-rename_cedb-2330091-20.interdiff.txt1.34 KBplach
#18 entity-rename_cedb-2330091-18.patch44.84 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es), 1 fail(s), and 0 exception(s). View
#18 entity-rename_cedb-2330091-18.interdiff.txt7.74 KBplach
#18 entity-rename_cedb-2330091-13.interdiff.txt40.02 KBplach
#13 entity-rename_cedb-2330091-13.patch52.46 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,763 pass(es), 1 fail(s), and 6 exception(s). View
#8 entity-rename_cedb-2330091-8.patch35.73 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es). View
#5 entity-rename_cedb-2330091-5.patch35.56 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#3 entity-rename_cedb-2330091-3.patch29.06 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#1 entity-rename_cedb-2330091-1.patch48.28 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-rename_cedb-2330091-1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

plach’s picture

Status: Active » Needs review
FileSize
48.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch entity-rename_cedb-2330091-1.patch. Unable to apply patch. See the log in the details link for more information. View

First attempt

Status: Needs review » Needs work

The last submitted patch, 1: entity-rename_cedb-2330091-1.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
29.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

wow

Status: Needs review » Needs work

The last submitted patch, 3: entity-rename_cedb-2330091-3.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
35.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Bogus patch

Status: Needs review » Needs work

The last submitted patch, 5: entity-rename_cedb-2330091-5.patch, failed testing.

yched’s picture

Oh yes please :-)

plach’s picture

Status: Needs work » Needs review
FileSize
35.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es). View

Forgot to rename the unit test class file.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

plach’s picture

Maybe we should move the class to the Drupal\Core\Entity\Sql namespace while we are at it?

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

Re #10: +1 from me. What do others think?

Berdir’s picture

The interface is already on there, makes sense to me.

What do we want to do with EntityDatabaseStorage now? It's unused and untested, so maybe we should just remove it instead of bothering to rename it?

plach’s picture

FileSize
52.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,763 pass(es), 1 fail(s), and 6 exception(s). View

Removed EntityDatabaseStorage and moved content entity storage and schema handler to the Sql namespace.

Status: Needs review » Needs work

The last submitted patch, 13: entity-rename_cedb-2330091-13.patch, failed testing.

Berdir’s picture

Looks like there's still one test entity type using that storage. Let's clean that up in a separate issue, sorry for the noise.

Berdir’s picture

Double-post. More noise.

Berdir’s picture

Opened #2332577: Remove EntityDatabaseStorage for that, might need some more discussion about the EntityApiInfoTest, so i think it's better to keep that separate.

plach’s picture

Status: Needs work » Needs review
FileSize
40.02 KB
7.74 KB
44.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es), 1 fail(s), and 0 exception(s). View

Reverted EntityDatabaseStorage changes. Attaching also the interdiff that I forgot in #13.

Status: Needs review » Needs work

The last submitted patch, 18: entity-rename_cedb-2330091-18.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
45.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,868 pass(es). View

Fixed unit tests

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, good to go. EntityDatabaseStorage is also already RTBC, but it's still better to keep it separate I think.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 71d2479 on 8.0.x
    Issue #2330091 by plach: Rename ContentEntityDatabaseStorage to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.