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.
Let's inform people if they have a problematic route variable.
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 1.83 KB | sun |
#43 | drupal8.routing-attributes-validation.43.patch | 3.27 KB | sun |
#40 | drupal8.routing-attributes-validation.40.patch | 2.77 KB | sun |
#35 | 2051877-35.patch | 5.3 KB | dawehner |
#32 | drupal-2051877-32.patch | 2.18 KB | dawehner |
Comments
Comment #1
dawehnerThere we go.
Comment #2
dawehnerComment #3
jibranDo we need tests for this?
Comment #4
dawehnerRight, just wanted some opinions first to be honest.
Comment #5
Crell CreditAttribution: Crell commentedA double foreach() in userspace along critical path concerns me. It feels like there ought to be an array_intersect() or similar approach where we can push everything down to C.
"Invalid route pattern variable" isn't self-descriptive. It should say why it's invalid, specifically that it's a reserved name.
Comment #6
pwolanin CreditAttribution: pwolanin commentedrelated to: #2052389: All elements added to the Request attributes should have a _ prepended unless they come from the path which would make this easier?
I'm not sure when this validation fires?
Seems to be missing some like 'theme' and 'drupal_menu_item', though as above I think we should prefix them. Then this might switch to white-listing _account and _raw_variables?
Comment #7
Crell CreditAttribution: Crell commenteddrupal_menu_item is temporary only for for the legacy router. It will go away. Don't worry about it.
Comment #8
dawehnerThis validation is fired when the routes are rebuild, which is basically on module enable and actions like saving a view.
Changes in the patch: Used array_intersect and added some test coverage.
Comment #9
Crell CreditAttribution: Crell commentedThrowing an exception during route rebuild worries me. If that's not caught and handled properly we're guaranteed to end up in an unstable, routes-don't-work state. That exception needs to be handled properly in RouteBuilder; I'd think "properly" here means "let every route but that one go through". Which I think means we can't use an exception.
Comment #10
dawehnerMh, we could write to watchdog instead. Especially now that account got renamed to _account, this is really an edge case,
so it seems fine for me, if it fails as early as possible.
Comment #11
jibrantestOnRouteBuildingValidVariables
/me runs
Comment #12
dawehnerThere we go.
Comment #13
Crell CreditAttribution: Crell commentedSkip-and-log seems reasonable to me, and less likely to asplode the site.
Comment #14
dawehnerI don't want to argue against personal, but people won't look at it at all, if you are honest.
This error will just happen, if you write code, so having a broken route definition, seems okay, because then you rebuild it and get the exception message, so you can fix it.
Comment #15
Crell CreditAttribution: Crell commentedDevel has needed a setting to dsm() any watchdog message for a long time. That's the solution to "people don't read logs". Someone needs to just go add it to devel. :-)
I think #14 is the wrong patch, though. :-)
Comment #16
dawehnerIt would be great if you could provide something else then feelings, but actual points, sorry.
Comment #17
dawehnerStill applies.
Let's not break anyone and log the error.
It would be cool to have this access checker for #2057607: The request does not contain the _account on exception pages (403/404).
Comment #18
Crell CreditAttribution: Crell commentedClose enough for government work.
Comment #19
alexpottCommitted bc0570c and pushed to 8.x. Thanks!
Comment #20
tstoecklerHope it's OK for me to re-open this one, since there's not that many comments so far.
I think the drupal_set_message() should be at least type 'warning'. Showing it in green seems a bit strange, IMO.
Comment #21
Crell CreditAttribution: Crell commentedI agree with that.
Comment #22
catchSorry I don't agree with a drupal_set_message() here - that completely ignores error_level settings so you'll potentially have site visitors seeing error messages that mean nothing.
Why not trigger_error - that handles both watchdog and dsm() if it's set up right for development, without being an in-your-face exception.
Comment #23
dawehnerGood idea!
Let's hope this is the right error level, but I don't have a clue.
Comment #24
tim.plunkettFixing tag
Comment #25
Crell CreditAttribution: Crell commentedThere are no situations in which trigger_error() is correct in object-oriented code[1]. Global engine state is absolutely not an OK API when you want self-contained testable objects. Logging is the real answer here. As I said above, showing logs in dsm() should be devel's job (or some other config setting). I can deal with the dsm() being in there since that config setting doesn't exist yet, but trigger_error() is not OK.
[1] I could possibly consider E_USER_DEPRECATED, but that's not something we ever use.
Comment #26
catchWhat world do we live in where drupal_set_message() is OK in OO code and trigger_error() isn't?
Comment #27
msonnabaum CreditAttribution: msonnabaum commentedAgree with catch.
There is no OOP argument for or against language level features. This is PHP, trigger_error() is a thing.
Comment #28
Crell CreditAttribution: Crell commented$_POST is a thing in PHP, too, that doesn't mean we're OK with using it. :-)
And dsm() is there only because we've not gotten it moved to something OO yet.
trigger_error() is the wrong tool here. logging is the right tool. We need nothing more than that *here*.
Comment #29
alexpottSo I committed this originally because I was happy that we were actually helping people by catching this and saying why. I preferred the patch when it was throwing an exception but I valued the fact that we were failing in a better way more.
But thinking again, we might be over-thinking this.
so the funny thing is actually logging instead of triggering an error or throwing an exception makes it more likely a module will end up on d.o that does this because the code still works when it should be broken.
Comment #30
dawehnerThat is a good point. We should fail as fast as possible so it never gets to the situation
Once the developer sees the broken routing system, rebuiling is needed and than the exception is shown.
Comment #31
catchException works for me - I'm fine with anything that results in a PHP error.
Comment #32
dawehnerNow we potentially have made all variants.
Comment #33
jibranI think everyone is happy now.
Comment #34
Crell CreditAttribution: Crell commentedUh. No, see comment #9 for why Exception is ungood.
(I spent about 4 patches trying to get theme() to throw an exception on an invalid theme hook rather than silently failing in Drupal 7. Every one ended up breaking the site unrecoverably. Eventually we went for watchdog-and-return-empty-string, which is what webchick and told me to do in the first place. She was right. The same logic applies here.)
Comment #35
dawehnerWe could store each exception, so we can still rebuild the routes ... Nevertheless I don't see why this is problematic. Alex described in detail the possible cases when the exception is thrown.
When you develop the code you will see it. I have no problem with forcing people to rebuild the router if they made a really uncommon mistake when defining router.
Comment #36
Crell CreditAttribution: Crell commentedAnd how do they rebuild the router if the router ends up broken and the admin/development/performance page won't load? Note: Drush doesn't ship with core, so Drush is not a sufficient answer.
This was a regular problem in Drupal 7. Let's not make it a problem in Drupal 8.
Comment #37
dawehnerDoes rebuilding the container triggers a rebuild of the router? I am just wondering but I see your point, we don't want to require silly developers to use drush.
Comment #38
Crell CreditAttribution: Crell commentedNo, container rebuild and router rebuild are different operations, and all are different from clearing the actual cache system. drupal_flush_all_caches() triggers all of them, plus others. That's the function called from the button on the performance page.
Comment #39
sunJust happened to run into the lack of proper error handling/reporting in this class, over in #2301873: Use ExtensionDiscovery to register test namespaces for running PHPUnit
Personally, I consider this particular condition as a code/logic error. I'm not able to see a difference between an invalid route definition like this and a PHP syntax error in a dynamic route builder class. In the latter case, the application will not be able to rebuild routes. Why should it in the former?
This particular case is about a code/logic error that is caused by a mistake of a developer. It should be made immediately visible to the developer/author. The application SHOULD fatal with an exception.
Hiding away a code/logic error into
dsm()
/watchdog()
has only one effect: Broken code that no one is aware of.A great example: #2301245: Entity system invokes non-existing theme hooks: "Theme hook $entity_type_id not found." — Most likely, that functional code/logic bug exists for 1-2 years in HEAD already, but no one noticed it.
If we really cannot agree on throwing an uncaught exception, then
trigger_error()
is indeed the most appropriate and correct tool.Comment #40
sunGiven the large amount of hidden code/logic errors revealed by #674108-38: ThemeManager::theme() does not trigger an error when a theme hook is not found - which is conceptually about the identical issue - let's at least move forward with proper error reporting here, too.
Attached patch switches to
trigger_error()
and adjusts the unit test accordingly.Comment #41
dawehnerMh, just in general it seems pointless to return a boolean if we validate multiple entries. should we just return the list of valid/invalid route names?
Comment #42
sunThe more important logic correction there is that the code in HEAD breaks the loop and returns upon encountering the first route that uses reserved variables, so any possible subsequent routes in the same collection are not reported.
I'm not sure what the return value is good for. I was tempted to remove the return value entirely, but I'm not familiar with this subsystem.
Comment #43
sunSans return value — doesn't appear to be part of the RouteSubscriber::alter event API anyway.
Comment #44
dawehner+1 for improving the actual message.
Comment #46
Crell CreditAttribution: Crell commentedNo. trigger_error() is a global state operation. It has no place in our current service architecture. Just inject the log service and use that instead.
Comment #47
dawehnerLogging will hide this error, and based on this makes the file itself pointless. This file was to improve the DX while developing code.
Beside from that here is a different point in favour of throwing an exception:
This code is from \Symfony\Component\Routing\RouteCompiler and clearly is a validation of the specified route. compile() is executed
as part of the MatcherDumper and we do not catch any exception here.
These two validations (especially the first one) will happen way more often than the one we talk about (SpecialAttributesRouteSubscriber).
From my perspective we are already throwing exceptions, so we already have a good chance that you have to rebuild using rebuild.php or whatever.
Comment #48
Crell CreditAttribution: Crell commentedAs I said in IRC, an exception is appropriate iff we catch it in an appropriate place and handle it appropriately. (By undoing/rolling back any rebuilds that have happened so that the routing system is not left in an unstable and unpredictable state.) Throwing an exception and hoping that someone catches it is not an appropriate way to handle it. We'd need to catch the exception in RouteBuilder and so... something useful with it.
Comment #49
dawehnerWe for sure can do quite a lot of useful things in case we catch the exception, like revert to a previous state of the dumped routes (note: we figured out earlier that holding the routes in memory is not a big deal).
Though hiding the exception is still a problem. Can we somehow let the exceptions fall through in some DEV enviroment and catch them and revert in production?
Comment #50
sunHow is
trigger_error()
related to global state? It triggers the PHP error handler. You may register a custom error handler, but that is done viaregister_error_handler()
.trigger_error()
does not affect global state.trigger_error()
is toregister_error_handler()
likefopen()
is tostream_wrapper_register()
.Why should we suddenly start to work around native error reporting of PHP core? Your custom error handler may log, it may show a message, and do whatever else you consider appropriate for an error.
The point of configurable error reporting is to configure whether errors are visible or not. If we start to work around and ignore the error reporting configuration, then the error reporting configuration becomes useless/pointless.
"Inject a logger and use that" is not an adequate replacement. That's logging, not error handling. You get a log message (somewhere), but no sign of an error.
Comment #51
Crell CreditAttribution: Crell commentedWhat do you mean hide the exception? Handle it properly. That may include logging and/or dsm()ing. An uncaught exception unrolls the entire stack and ends the request prematurely. We don't want that. :-)
Comment #52
msonnabaum CreditAttribution: msonnabaum commentedTotally agreed with #50.
Comment #53
tim.plunkettI also agree with #50.
Comment #54
dawehnerWhat sun said in #50!
Comment #55
tim.plunkettComment #56
Crell CreditAttribution: Crell commentedAs dawehner said in #47, "we're already throwing exceptions" (via Symfony). So why are we not doing the same, throwing an exception *and catching it intelligently*?
Comment #57
tim.plunkett@dawehner was proposing throwing *uncaught* exceptions. Everyone else is fine with trigger_error().
@Crell, you are the only person to say that we should catch the exceptions. Pray tell, what would we do inside that catch statement?
Comment #58
sun@Crell: Throwing an exception cancels the processing of the entire route collection; any subsequent routes are not processed.
As you mentioned yourself, it is nice to not completely melt-down with a fatal error, if we can trigger a proper error instead. This targets a one-time/single exposure situation anyway - as soon as developers understood that their code is problematic, it's unlikely that they're going to repeat the mistake.
Can we just simply get this in, please?
Comment #59
sunRegardless of what's going to happen here, this entire debate and the idea of (ab)using pure logging for errors instead of triggering proper PHP errors reminded me to get back to #652394: Aggressive watchdog message assertion — it's going to be part of a different issue though.
Comment #60
catchtrigger_error() is the right thing to use here, it's disappointing to see constant FUD around using it and people suggesting ugly and incorrect workarounds just to avoid something they don't like aesthetically.
We should throw exceptions when something needs to be fatal, but there's a reason that notices and warnings exist, and trigger_error() is the correct way to handle errors at that level. If you don't like trigger_error(), then campaign for notices and warnings to be removed from PHP too.
Comment #61
catchPutting #43 back to RTBC.
Comment #63
alexpottI think that using trigger_error is correct here too... we don't get a half built router, we inform developers exactly what is going on.
Committed 9523119 and pushed to 8.x. Thanks!