Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
SpecialAttributesRouteSubscriber::onAlterRoutes() is documented as returning TRUE or FALSE on success or failure.
It simply calls SpecialAttributesRouteSubscriber::alterRoutes() which does not return a value.
I know this because I decided to run PHPUnit with strict mode on, and fix non-strict tests, and ran into a problem that wouldn't be a problem if tests were strict by default.
Proposed resolution
Documentation is incorrect, event methods don't return anything. Remove the documentation and don't return anything from the subscribe method.
Remaining tasks
Comment | File | Size | Author |
---|---|---|---|
#5 | 2364467-5.patch | 5.32 KB | neclimdul |
#5 | 2364467-5.interdiff.txt | 4.81 KB | neclimdul |
#1 | 2364467_1_pass.patch | 5.25 KB | Mile23 |
#1 | 2364467_1_fail.patch | 5.25 KB | Mile23 |
Comments
Comment #1
Mile23Here are two patches. Both are better tests. One will pass by testing the current behavior, and one will fail by testing the documented behavior.
Comment #4
Mile23Basically, just need a judgement on which piece of the machine is wrong: Docs, alterRoutes(), or onAlterRoutes().
Comment #5
neclimdulReroll and going to make a judgement/state opinion.
The return from the onAlterRoutes is thrown away by the eventDispatcher so it should go.
Also, I used the straight objects instead of mocking them since we aren't using the mocks. Route and RouteBuildEvent are essentially data objects so I'm don't see a need to mock them and the subscriber being mocked is the object we're testing and we aren't asserting any logic with the mock.
Comment #6
neclimdulderp. testbot
Comment #7
Mile23Oo... Deja vu.
Agreed that the event dispatcher doesn't really need a return value, so it's important to have the exception.
And since we have a test method that asserts the exception, we can also have another test method that just says
assertNull()
.Setting to RTBC even though it was my patch last October. Feel free to get another opinion if desired.
Comment #10
Mile23RTBC back from re-test.
Comment #11
dawehnerThe patch is green.
Comment #12
neclimdulNote: Failure was something in Locale or something random. Forgot to save the test run.
Comment #13
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed c1b4c03 and pushed to 8.0.x. Thanks!