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.
I want to unit test my own Views handlers, but Views' base classes depend on the global scope too much. I do not have time to write a unit test for \Drupal\views\Plugin\views\HandlerBase
myself, but I am hoping that a green test result and my feedback about my contrib tests passing will be enough to get this issue fixed.
Comment | File | Size | Author |
---|---|---|---|
#65 | vdc-2174021-65.patch | 6.39 KB | dawehner |
#62 | drupal-2174021-62.patch | 6.4 KB | dawehner |
#59 | drupal-2174021-59.patch | 6.61 KB | dawehner |
#56 | drupal_2174021_56.patch | 7.12 KB | Xano |
Comments
Comment #1
XanoInitial patch. There will likely be more changes.
Comment #2
dawehnerCertainly we have a lot of tasks to solve before we can unit tests these classes, but this is some sort of good start.
Comment #3
XanoI added wrappers to get the module handler and Views data services, and replaced the calls to procedural string manipulation functions with calls to their OOP replacements.
Comment #5
Xano3: drupal_2174021_3.patch queued for re-testing.
Comment #7
XanoComment #8
damiankloip CreditAttribution: damiankloip commentedI'm sorry but I don't see how having these 'wrapper' methods makes testing any easier?
Comment #9
XanoIsn't the policy to wrap calls to \Drupal in protected methods, store the retrieved services in protected properties, and test by building a custom container in the test (#2171315: Ensure a NULL container in UnitTestCase::setUP proves that's a bad idea) or mock the class under test and let the wrapper methods return mocked services rather than calling \Drupal::service()?
Comment #10
damiankloip CreditAttribution: damiankloip commentedWhat's wrong with mocking the container? IMO that is better than mocking the class under test. I don't really like that unless we are testing an abstract class.
Comment #11
dawehnerOn the longrun I would suggest to use setter injection here, which we could do in our plugin/handler manager automatically
Comment #12
damiankloip CreditAttribution: damiankloip commentedYeah, we talked about that in irc. As I suggested it originally I am certainly in favour of that! :)
Comment #13
XanoI agree on using setter injection if we really can't use constructor injection, but seeing as such setters are only relevant for tests and this patch does not actually introduce a test, I'm honestly not sure whether we should add those setters in this patch.
Comment #14
XanoAdding implementation logic to plugin managers sounds like a Very Bad Idea™, especially because that would also require us to extend interfaces with setters, which completely defeats the purpose of having those interfaces in the first place (separation of contract and implementation).
Comment #15
damiankloip CreditAttribution: damiankloip commentedNot sure why that comes into it? Which interfaces?
A. What happens in views plugin manager getHandler method is its own business.
B. Views doesn't really use interfaces for handlers.
Comment #16
XanoIf the manager calls methods on plugins, those plugins must implement an interface that defines those methods, otherwise we would be relying on implementation details. Setter methods for dependencies should not belong on interfaces, as dependencies are more often than not implementation details, and we can't couple those to an interface.
Comment #17
damiankloip CreditAttribution: damiankloip commentedWell we don't even have to do this in the plugin manager. End of argument.
Comment #18
XanoBack to the last patch, what is your verdict?
Comment #19
XanoRe-roll.
Comment #20
dawehnerXano already had an opinion about the following suggestion, @damian what do you think about it?
Comment #21
XanoSo much for decoupling interfaces and implementations, but if this it what it takes to make this unit-testable, let's get it over with.
Comment #22
damiankloip CreditAttribution: damiankloip commentedWe don't really use an interface for handlers, its all about extending the base class - which this does. If for some insane reason you wanted to write a views handler from scratch, You don't get this anyway.
Comment #23
dawehnerSo damian, xano, can we get this RTBC?
Comment #24
XanoSure. I just don't think I'm allowed to RTBC this, seeing as I wrote most of the code. The honor is therefore all yours :)
Comment #25
dawehnerRules are there to be broken :p
Comment #26
XanoVictory!
Comment #27
alexpottvdc-2174021.patch no longer applies.
Comment #28
XanoRe-roll.
Comment #29
XanoRTBC, provided the tests pass.
Comment #30
Sutharsan CreditAttribution: Sutharsan commented'Needs reroll' tag removed.
Comment #31
alexpottdrupal_2174021_28.patch no longer applies.
Comment #32
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #33
dawehnerRertbc
Comment #34
alexpott2174021-32.patch no longer applies.
Comment #35
XanoRe-roll. RTBC, provided that the tests pass.
Comment #36
XanoRTBC, as this was the reject:
Comment #37
Sutharsan CreditAttribution: Sutharsan commentedComment #38
alexpottdrupal_2174021_35.patch no longer applies.
Comment #39
XanoRe-roll. RTBC, under the condition that the tests pass, because the rejects were only about injecting the module handler into
ViewsHandlerManager
, which is already done in HEAD these days.The rejects:
Comment #40
XanoComment #41
Alumei CreditAttribution: Alumei commentedComment #42
alexpottAre these ever serialised? Do we need to worry about serialising the data and moduleHandler?
Comment #43
XanoAs far as I know, serializing plugin instances is a Bad Idea™ and unsupported by design. If you want to reproduce an instance later, save its ID and configuration instead.
Comment #44
dawehnerI kind of fear that the views forms will contain references to the view object and so also to the instances of the plugins.
Comment #45
XanoWe can either not store dependencies in plugin class properties (easy solution), or prevent view objects from serializing the plugins they contain.
Comment #46
dawehnerOh, Drupal\Core\Plugin\PluginBase already is serializable-aware.
Comment #47
dawehnerSo for me the patch seems to be safe.
Comment #48
scor CreditAttribution: scor commented39: drupal_2174021_39.patch queued for re-testing.
Comment #50
XanoRe-roll, because of recent additions to Unicode. Reject log:
Comment #51
XanoRTBC, under the condition that the tests pass.
Comment #53
Xano50: drupal_2174021_50.patch queued for re-testing.
Comment #54
XanoThe tests failed because of a random segmentation fault. RTBC'ing as per #33 and #51.
Comment #55
alexpottdrupal_2174021_50.patch no longer applies.
Comment #56
XanoRe-roll. Reject:
Comment #57
XanoRTBC as per #54, under the condition that the tests pass.
Comment #59
dawehnerRerolled.
Comment #60
dawehner.
Comment #62
dawehnerReroll.
Comment #63
XanoComment #65
dawehnerReally ...
Comment #66
XanoComment #67
alexpottCommitted 9696e51 and pushed to 8.x. Thanks!
Fixed in commit.