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
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff_27_31.txt | 407 bytes | Arantxio |
#31 | 3209605-31.patch | 1.26 KB | Arantxio |
|
Issue fork drupal-3209605
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:
- 3209605-add-new-hook changes, plain diff MR !575
Comments
Comment #3
daffie CreditAttribution: daffie commentedComment #4
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedComment #5
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedComment #6
daffie CreditAttribution: daffie commented@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.
Comment #7
LendudeSounds 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)
Comment #8
daffie CreditAttribution: daffie commented@Lendude: Thank you for your review! I have added the requested improvements for the testing.
Comment #9
LendudeNothing more to add from my end, looks good.
Comment #10
jibranWhy are we doing it on ViewsData level? Why can't we do it in views query plugin? Or in the views relationship plugin?
Comment #11
daffie CreditAttribution: daffie commentedWhen 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.
Comment #12
jibran...and the *SQL* database table structure...in the *SQL* database. Views in cores has a lot of
SqlEntityStorageInterface
checks.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:
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?
Comment #13
daffie CreditAttribution: daffie commentedYes, 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?
Comment #14
jibranViewsData
holds the mapping between entity fields and DB tables & columns (along with plugin info) butViewsData
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.Comment #16
LendudeHmm 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.
Comment #20
daffie CreditAttribution: daffie commentedLets 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.
Comment #21
ArantxioPer 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.
Comment #22
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedfix ccf error #21
Comment #23
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedSorry for the wrong patch
Comment #25
daffie CreditAttribution: daffie commentedI do not know why this change was added, but it can be removed.
Comment #26
Arantxio@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:
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.
Comment #27
ArantxioI've removed the subproject part of the patch.
Comment #28
daffie CreditAttribution: daffie commentedThe 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.
Comment #29
daffie CreditAttribution: daffie commentedComment #30
quietone CreditAttribution: quietone at PreviousNext commentedJust a small thing.
The service name should be lower case, "views.views_data".
Comment #31
ArantxioI've made the service name lower case.
@quietone thank you for your comment.
Comment #32
daffie CreditAttribution: daffie commentedThe remark of @quietone has been addressed.
Back to RTBC.
Comment #35
xjmI 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:
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!