The mysql.views.cast_sql service depends on the views module and exists when views is not installed

Created on 12 December 2024, 7 months ago

Problem/Motivation

The MySQL module declares the following service:

services:
  mysql.views.cast_sql:
    class: Drupal\mysql\Plugin\views\query\MysqlCastSql
    public: false

The class has the following definition:

use Drupal\views\Plugin\views\query\CastSqlInterface;

/**
 * MySQL specific cast handling.
 */
class MysqlCastSql implements CastSqlInterface {

But the module does not (and should not) depend on views.

Steps to reproduce

  1. Install minimal drupal on MySQL
  2. Do \Drupal::service('mysql.views.cast_sql')

This causes problems for anything that iterates over services to find things out.

Proposed resolution

Move to a service provider that only adds the service in when views is installed.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

11.1 πŸ”₯

Component

database system

Created by

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @alexpott
  • πŸ‡·πŸ‡΄Romania amateescu

    Maybe we could improve \Drupal\Core\DependencyInjection\Compiler\BackendCompilerPass instead, and remove the backend-specific service from the container if the main service doesn't exist.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    @amateescu but without views installed we have no idea that mysql.views.cast_sql exists to override views.cast_sql

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
  • Pipeline finished with Failed
    7 months ago
    Total: 575s
    #367931
  • πŸ‡·πŸ‡΄Romania amateescu

    @alexpott, but we do know that mysql.views.cast_sql is a backend override service because it's tagged with backend_overridable, and then we just need to split the backend name from its ID to find out the main service ID.

    I'm not sure that MR was meant for this issue though :)

  • Merge request !10555Only add the service when views is around β†’ (Closed) created by alexpott
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    alexpott β†’ changed the visibility of the branch 3493595-the-mysql.views.castsql-service to hidden.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    @amateescu we only know that it is an override once the views service exists. This is about sites without views installed. The MySQL service is not tagged.

    Fixed the MR.

  • Pipeline finished with Failed
    7 months ago
    Total: 554s
    #368525
  • πŸ‡·πŸ‡΄Romania amateescu

    Ohh.. that's right, somehow I didn't realize that only the main service is tagged :)

    The kernel test failure seems to be caused by this change.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    The kernel failure occurred because before we had two services in services.yml files implementing \Drupal\views\Plugin\views\query\CastSqlInterface and now we have 1 - ie. the one in views.services.yml so now it errors. The best fix is to add the missing alias. The fact this is in a plugin directory is odd because it is a backend overridable service and it only ever accessed in real runtime code by doing \Drupal::service('views.cast_sql') so it's not a plugin at all. Anyhow that's not something this issue needs to resolve.

  • First commit to issue fork.
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    I only rebased to get the test-only feature to run

    1) Drupal\Tests\mysql\Kernel\mysql\ViewsTest::testViewsService
    Failed asserting that false is identical to true.
    /builds/issue/drupal-3493595/core/modules/mysql/tests/src/Kernel/mysql/ViewsTest.php:33
    /builds/issue/drupal-3493595/core/lib/Drupal/Core/DrupalKernel.php:1363
    /builds/issue/drupal-3493595/core/lib/Drupal/Core/DrupalKernel.php:901
    /builds/issue/drupal-3493595/core/lib/Drupal/Core/DrupalKernel.php:505
    /builds/issue/drupal-3493595/core/tests/Drupal/KernelTests/KernelTestBase.php:378
    /builds/issue/drupal-3493595/core/tests/Drupal/KernelTests/KernelTestBase.php:258
    /builds/issue/drupal-3493595/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificKernelTestBase.php:48
    /builds/issue/drupal-3493595/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificDatabaseTestBase.php:24
    FAILURES!
    Tests: 1, Assertions: 1, Failures: 1.
    

    So test coverage is good
    Issue summary is clear

    For the comment in #12 is that something that should be opened in a follow up?

    Rest of the code (from what I can tell) seems fine to me.

  • Pipeline finished with Failed
    6 months ago
    Total: 656s
    #379157
    • longwave β†’ committed 778c9bd5 on 11.x
      Issue #3493595 by alexpott, amateescu: The mysql.views.cast_sql service...
  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    I don't think it's worth moving the class to a different namespace so no followup is needed. Also not sure it's worth backporting this so let's just stick to 11.x unless there is another reason?

    Committed 778c9bd and pushed to 11.x. Thanks!

  • Status changed to Fixed 5 months ago
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024