Problem/Motivation

Interacting with entity storage should never use raw SQL, as there is no guarantees about the schema of the table.
It should instead use the entity query API

Edit: this is actually dead code, there's no calls to it, so we can remove it.

Steps to reproduce

Proposed resolution

Use entity queries instead of raw SQL

Remaining tasks

User interface changes

API changes

Data model changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Title: Don't use SQL queries when dealing with entities in the storage handler » Remove SitewideAlertStorage class, the code is unused
Issue summary: View changes

larowlan’s picture

Status: Active » Needs review
larowlan’s picture

Assigned: larowlan » Unassigned
ChrisSnyder’s picture

Status: Needs review » Needs work

I don't think all of this code is dead. For example, \Drupal\sitewide_alert\Controller\SitewideAlertController::revisionOverview, makes a call to \Drupal\sitewide_alert\SitewideAlertStorage::revisionIds. There may be some additional refactoring to remove the SitewideAlertStorage class

larowlan’s picture

Indeed, you're correct.

Added some tests and it fails as expected

1) Drupal\Tests\sitewide_alert\Functional\SitewideAlertControllerTest::testAlertRevisions
Exception: Error: Call to undefined method Drupal\Core\Entity\Sql\SqlContentEntityStorage::revisionIds()
Drupal\sitewide_alert\Controller\SitewideAlertController->revisionOverview()() (Line: 122)

Reworking

larowlan’s picture

Status: Needs work » Needs review

Pushed some code to use the entity query API (use of raw SQL for entities is not supported), tests now passing.

ChrisSnyder’s picture

ChrisSnyder’s picture

Status: Needs review » Fixed

  • chrissnyder committed 804eeda on 2.x authored by larowlan
    Issue #3261638 by larowlan, chrissnyder: Remove SitewideAlertStorage...

Status: Fixed » Closed (fixed)

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