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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

Status: Active » Needs review
FileSize
5.25 KB
5.25 KB

Here are two patches. Both are better tests. One will pass by testing the current behavior, and one will fail by testing the documented behavior.

Status: Needs review » Needs work

The last submitted patch, 1: 2364467_1_pass.patch, failed testing.

The last submitted patch, 1: 2364467_1_fail.patch, failed testing.

Mile23’s picture

Basically, just need a judgement on which piece of the machine is wrong: Docs, alterRoutes(), or onAlterRoutes().

neclimdul’s picture

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

neclimdul’s picture

Status: Needs work » Needs review

derp. testbot

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2364467-5.patch, failed testing.

Status: Needs work » Needs review

neclimdul queued 5: 2364467-5.patch for re-testing.

Mile23’s picture

RTBC back from re-test.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The patch is green.

neclimdul’s picture

Note: Failure was something in Locale or something random. Forgot to save the test run.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed c1b4c03 on 8.0.x
    Issue #2364467 by Mile23, neclimdul: SpecialAttributesRouteSubscriber::...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.