Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
#2899226: Provide API for contrib modules finally added a clean API for integrating with VBO to its D8 version.
We should take advantage of this opportunity right away and provide our own integration. It's really simple, and Graber already provided a sandbox with a working prototype.
Comment | File | Size | Author |
---|---|---|---|
#10 | 2899678-10--vbo_intergation.patch | 7.02 KB | drunken monkey |
|
Comments
Comment #2
drunken monkeyThis should do it.
Some open questions, though:
provideViewData()
, makegetEntityFromRow()
an instance method (i.e., non-static) and with that actually have a chance to apply the relationship we get passed. Otherwise, I don't see any way to incorporate the relationship. (VBO's own implementation actually uses an instance method, but calls it statically, which is obviously a bug.)NULL
from the getter, or do we have to throw an exception? In the Search API, it's of course easily possible to have a non-entity result row, and this is even more true if a relationship is passed (the relationship in question might refer to a completely different datasource than the result row has), so VBO being able to handle this is pretty much a requirement. (VBO's own implementation throws anException
in some cases, but not sure how it would then react to that.)Also, I used this opportunity to move
SearchApiQuery::getEntityTypes()
to the index class. It's useful enough for that, I think.Comment #3
drunken monkeyAlso added a change record for the
getEntityType()
move.Comment #4
borisson_Not sure about the descriptions here. Otherwise this looks great.
I also think we maybe should rename the eventSubscriber, ViewsBulkOperationsEventSubscriber carries more meaning.
If you disagree, this is rtbc for me.
Comment #5
Graber CreditAttribution: Graber as a volunteer commentedHi,
The questions:
getEntityFromRow()
method as I don't know where it should belong (I made it a public static method and left it in the processor service in one of the last commits). If I can make the API more flexible somehow, let me know, you can always pass a service object and method to the callable if needed.EntityInterface
and passes only those to the selected action. For a non-entity the output showed to the user will be "Skipped".I also had some doubds about not using
$event->getProvider()
and type-hinting I mentioned in an email, but the first is insignificant and the second - I like your solution withif (class_exists(ViewsBulkOperationsEvent::class)) {
more, I'm going to update examples in the API docs.Thanks,
Graber
Comment #6
drunken monkeyThanks a lot for spotting that! Those were left-overs from when this was in its own module (where it was pretty clear what the event subcriber was doing). Updated the description to be, hopefully, more helpful.
This would violate the (debatable, but at least in this module mostly adhered-to) rule about duplicating the namespace in the class name. Since the namespace is
Vbo
, I thinkEventSubscriber
is sufficient. We could rename the namespace toViewsBulkOperations
, though, if you thinkVbo
isn't clear enough. (Though we now also mention the complete module name in the class description.)Another In principle, however, I must agree that just the bare class name looks a bit "wrong". But it's how we're handling namespaces in the whole module, there are lots of other such examples, where the unqualified class name looks rather unhelpful.
The only other alternative I could think of would be to move it to a different sub-namespace, for non-plugin contrib module integration in general. I.e., something like
\Drupal\search_api\Contrib\ViewsBulkOperationsEventSubscriber
(or…\VboEventSubscriber
). Would you find that better? Having a namespace where there is little chance a second class will ever be added is a bit unsatisfactory anyways.Hm, OK, that's a) pretty logical and b) unfortunate. Without a view object, we unfortunately can't resolve the relationship. (I see how you've solved it in your processor, which makes sense, but unfortunately the Search API stores it's object structure differently.)
However, this sounds like you don't actually need the getter when called from the admin form (which makes sense), so we could maybe just set that one conditionally? That would resolve the problem for us, though it might look a bit funny in the
provideViewsData()
method. Otherwise, we'd need you to pass the view object to the getter.Comment #7
drunken monkeyComment #8
borisson_I agree that we haven't done this before, but since this is integration with another module I just wanted to make sure that we convey sufficient meaning.
I think that the alternative with
\Contrib\ViewsBulkOperationsEventSubscriber
in the namespace is a better solution. As that should convey sufficient meaning.Comment #9
Graber CreditAttribution: Graber as a volunteer commentedI had a feeling I may need to provide the current
ViewsExecutable
object to the row getter. Included in 8.x-1.x-dev, will soon add it to alpha1 too.Comment #10
drunken monkeyOK, then let's go with that.
OK, great, thanks! Added code handling relationships. Seems to work well enough, even though it's a bit hack-y. However, the alternatives would have been larger refactoring or copy-pasting of dozens of lines of code.
Please test/review!
Comment #12
borisson_This looks solid, I haven't tested it but based on the patch this could go in.
Comment #13
drunken monkeyThanks a lot for reviewing!
Graber, do you still want to test and/or review this latest version? Otherwise, I too think we can commit this.
Comment #14
Graber CreditAttribution: Graber as a volunteer commentedCode review OK, all working fine on my test search_api view as well.
Thanks!
Comment #16
drunken monkeyGreat to hear, thanks for the feedback!
Committed.
Thanks again, everyone! Great we finally got Search API and VBO working together!