Problem/Motivation

The database driver for MongoDB needs to change the views data after all calls to the hook_views_data_alter() have run. MongoDB stores all entity data for an entity instance in a single document and not in multiple tables as relational databases do. Now there is no such possibility.

Proposed resolution

Make the service views.views_data backend overridable.

Remaining tasks

User interface changes

None

API changes

The service views.views_data is now backend overridable.

Data model changes

None

Release notes snippet

None

Issue fork drupal-3209605

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

daffie created an issue. See original summary.

daffie’s picture

Status: Active » Needs review
KapilV’s picture

KapilV’s picture

FileSize
1.75 KB
daffie’s picture

@KapilV: Thank you for working on this issue. Could you in the future when an issue the new merge request (MR) functionality, add your changes to that MR and not add a patch file. The changes are between the MR and a patchfile are difficult to follow.

Lendude’s picture

Sounds like a fair request to me.

Already looks pretty good, mostly wondering about the test coverage here. We currently only test that the hook is being called, but since we are not using the out of the box _alter functionality here (and it makes sense not to), should we not add a test to see that we can actually alter the Views data using the hook and that the expected order of calls is being maintained (so normal alter first, then the driver provider alter)

daffie’s picture

@Lendude: Thank you for your review! I have added the requested improvements for the testing.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

Nothing more to add from my end, looks good.

jibran’s picture

Why are we doing it on ViewsData level? Why can't we do it in views query plugin? Or in the views relationship plugin?

daffie’s picture

Why are we doing it on ViewsData level? Why can't we do it in views query plugin? Or in the views relationship plugin?

When a user builds a view in the browser, he or she does so from the entity perspective. The ViewsData is the translation layer between the entity perspective and the database table structure in which the data is stored in the database. When a view is executed the views query plugin collects and combines all the query parts from all the used views handlers. The handlers use the ViewsData build their part of the query. It is designed in this way to make a very complicated process maintainable and extensible for programmers. Moving the required changes to the ViewsData for the MongoDB database driver to the views query plugin or the views relationship plugin would making the maintainability and the extensibility of the views module for MongoDB a lot harder if not almost impossible.

jibran’s picture

Status: Reviewed & tested by the community » Needs review

The ViewsData is the translation layer between the entity perspective and the database table structure in which the data is stored in the database.

...and the *SQL* database table structure...in the *SQL* database. Views in cores has a lot of SqlEntityStorageInterface checks.

Moving the required changes to the ViewsData for the MongoDB database driver to the views query plugin or the views relationship plugin would making the maintainability and the extensibility of the views module for MongoDB a lot harder if not almost impossible.

Harder yes but not impossible, but the way it is done in the Views module is already harder, query, join, and relationships plugins. If that is still too hard then why not override views.views_data service and extend \Drupal\views\ViewsData.

IMO the problem is the following change:

  public function __construct(CacheBackendInterface $cache_backend, ConfigFactoryInterface $config, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager) {
  public function __construct(CacheBackendInterface $cache_backend, ConfigFactoryInterface $config, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager, Connection $connection) {

As you pointed out ViewsData is a map IOW a domain object and adding DB connection changes it form that.

Setting it to NR for more discussion. @Lendude what do you think about this point?

daffie’s picture

Yes, overriding the views.views_data service and extending \Drupal\views\ViewsData is also an option. To me the MR is a cleaner solution, in line with how the views module works and that is why I would like it committed to core.

@jibran: What does "map IOW a domain object" mean? And what does "IOW" stand for?

jibran’s picture

ViewsData holds the mapping between entity fields and DB tables & columns (along with plugin info) but ViewsData is just a domain object which means implementation details doesn't matter so instead of injecting DB connection it should be extended to accommodate the requirements of the other DB driver.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Lendude’s picture

Hmm looking at this again, I'm going to agree with @jibran on this. I do agree that using a hook is cleaner than overriding views.views_data, but injecting the DB connection does feel like a bit too much to accomplish this.

Overriding views.views_data also allows you to act on any joins done in \Drupal\views\ViewsData::processEntityTypes which the hook currently misses.

The old skool solution of giving you module a big weight would probably also work to ensure you are altering last, but that feels even less clean than the other options.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

daffie’s picture

Status: Needs review » Needs work

Lets do the by @jibran in comment #12 suggested solution and backend override the 'views.views_data' service.
A good example how to fix this is in #3209931: The service "cache_tags.invalidator.checksum" and the service "cache.backend.database" should be backend_overridable.

Arantxio’s picture

Status: Needs work » Needs review
FileSize
1.26 KB

Per request of @daffie, I've created a patch for the suggestions posted in comment #12.
And i've added a new file which tests the added tag.

Nitin shrivastava’s picture

fix ccf error #21

Nitin shrivastava’s picture

Sorry for the wrong patch

Status: Needs review » Needs work

The last submitted patch, 23: 3209605-23.patch, failed testing. View results

daffie’s picture

+++ b/core/modules/views/views.services.yml
@@ -59,6 +59,8 @@ services:
diff --git a/core/themes/gin b/core/themes/gin

diff --git a/core/themes/gin b/core/themes/gin
new file mode 160000

new file mode 160000
index 0000000000..0a89ac8733

index 0000000000..0a89ac8733
--- /dev/null

--- /dev/null
+++ b/core/themes/gin

+++ b/core/themes/gin
+++ b/core/themes/gin
@@ -0,0 +1 @@

@@ -0,0 +1 @@
+Subproject commit 0a89ac873395ae9ecff7474a4685a305463c9ac6
 

I do not know why this change was added, but it can be removed.

Arantxio’s picture

Status: Needs work » Needs review

@nitin Thank you for the patch, as I had to travel I haven't seen that it failed on the check.

Few things I would like to comment on is the following:

  1. +++ b/core/themes/gin
    @@ -0,0 +1 @@
    +Subproject commit 0a89ac873395ae9ecff7474a4685a305463c9ac6
    

    This part is not necessary and should not be added to the patch. If it is due to your project setup please try to remove it before posting the patch.

  2. In comment #22 you added a interdiff which contained a lot of changes that were also not in the patch. Please also try to avoid this as it will be unclear what your exact changed are.
Arantxio’s picture

I've removed the subproject part of the patch.

daffie’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

The chosen solution is the one from @jibran (comment #12).
The service views.views_data is now backend overridable.
There is testing added.
I have updated the IS and the CR.
For me it is RTBC.

daffie’s picture

Title: Add new hook DATABASEMODULE_views_data_DATABASEDRIVER_alter to the views module » Make the service views.views_data backend overridable
quietone’s picture

Status: Reviewed & tested by the community » Needs work

Just a small thing.

+++ b/core/modules/views/tests/src/Kernel/ViewsDataTest.php
@@ -0,0 +1,20 @@
+ * Tests the service Views.Views_data.

The service name should be lower case, "views.views_data".

Arantxio’s picture

Status: Needs work » Needs review
FileSize
1.26 KB
407 bytes

I've made the service name lower case.
@quietone thank you for your comment.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The remark of @quietone has been addressed.
Back to RTBC.

  • xjm committed f25a0ab6 on 10.1.x
    Issue #3209605 by daffie, Arantxio, jibran, Lendude, quietone: Make the...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

I was not familiar with this service tag, but the change record on backend-overridable services explains it quite well. The feedback from @Jibran and @Lendude was quite helpful, and @Lendude's signoff as a subsystem maintainer lends credence to this being the correct approach. I also made some improvements to the change record to better explain the purpose of this change (and to reference the original CR on the service tag).

I closed the MR since it was for a previous approach that is no longer used.

I made the following small grammar fixes on commit:

diff --git a/core/modules/views/tests/src/Kernel/ViewsDataTest.php b/core/modules/views/tests/src/Kernel/ViewsDataTest.php
index aea1bf88d6..5e40baeb25 100644
--- a/core/modules/views/tests/src/Kernel/ViewsDataTest.php
+++ b/core/modules/views/tests/src/Kernel/ViewsDataTest.php
@@ -10,7 +10,7 @@
 class ViewsDataTest extends ViewsKernelTestBase {
 
   /**
-   * Test that the service "views.views_data" is backend overridable.
+   * Tests that the service "views.views_data" is backend-overridable.
    */
   public function testViewsViewsDataIsBackendOverridable() {
     $definition = $this->container->getDefinition('views.views_data');

I removed credit for the broken patch from @Nitin shrivastava (since they were not able to come back and post a fixed version). If you need assistance with the patch creation process, I recommend the #contribute channel in the Drupal community Slack.

I also removed credit for the patch when an MR already existed from @KapilV. It's important to read the issue carefully and build on the work of other contributors, rather than attaching patches that fragment the review process and do not build on the work of previous contributors.

Committed #31 to 10.1.x, and published the change record. Thanks!

Status: Fixed » Closed (fixed)

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