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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review
FileSize
48.28 KB

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

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

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

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

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

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

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.