Make the service views.views_data backend overridable

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)

Nothing more to add from my end, looks good.

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

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?

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.

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.

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.

fix ccf error #21

Sorry for the wrong patch

@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.

I've removed the subproject part of the patch.

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".

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

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!