Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
1.88 KB

There we go.

dawehner’s picture

FileSize
0 bytes
2.42 KB
jibran’s picture

Do we need tests for this?

dawehner’s picture

Issue tags: +Needs tests, +WSCCI

Right, just wanted some opinions first to be honest.

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
@@ -0,0 +1,57 @@
+    foreach ($event->getRouteCollection()->all() as $route) {
+      foreach ($route->compile()->getVariables() as $variable_name) {

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

+++ b/core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
@@ -0,0 +1,57 @@
+          throw new \InvalidArgumentException(String::format('Invalid route pattern variable: @variable', array('@variable' => $variable_name)));

"Invalid route pattern variable" isn't self-descriptive. It should say why it's invalid, specifically that it's a reserved name.

pwolanin’s picture

Status: Needs review » Needs work

related 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?

Crell’s picture

drupal_menu_item is temporary only for for the legacy router. It will go away. Don't worry about it.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.77 KB
6.57 KB

I'm not sure when this validation fires?

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

Crell’s picture

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

dawehner’s picture

Mh, 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.

jibran’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/SpecialAttributesRouteSubscriberTest.phpundefined
@@ -0,0 +1,118 @@
+  public function testONRouteBuildingValidVariables(Route $route) {

testOnRouteBuildingValidVariables
/me runs

dawehner’s picture

Status: Needs work » Needs review
FileSize
849 bytes
6.57 KB

There we go.

Crell’s picture

Skip-and-log seems reasonable to me, and less likely to asplode the site.

dawehner’s picture

FileSize
3.74 KB
40.9 KB

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

Crell’s picture

Devel 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. :-)

dawehner’s picture

I think #14 is the wrong patch, though. :-)

It would be great if you could provide something else then feelings, but actual points, sorry.

dawehner’s picture

FileSize
3.16 KB
6.97 KB

Still 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).

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Close enough for government work.

alexpott’s picture

Title: Prevent people from using invalid route parameters » Log error when people use invalid route parameters
Status: Reviewed & tested by the community » Fixed

Committed bc0570c and pushed to 8.x. Thanks!

tstoeckler’s picture

Status: Fixed » Needs review
FileSize
1.07 KB

Hope it's OK for me to re-open this one, since there's not that many comments so far.

+++ b/core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
@@ -0,0 +1,63 @@
+    foreach ($event->getRouteCollection()->all() as $route) {
+      if ($not_allowed_variables = array_intersect($route->compile()->getVariables(), $special_variables)) {
+        $placeholders = array('@variables' => implode(', ', $not_allowed_variables));
+        drupal_set_message(String::format('The following variables are reserved names by drupal: @variables', $placeholders));

I think the drupal_set_message() should be at least type 'warning'. Showing it in green seems a bit strange, IMO.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I agree with that.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.32 KB

Good idea!

Let's hope this is the right error level, but I don't have a clue.

tim.plunkett’s picture

Issue tags: +MenuSystemRevamp

Fixing tag

Crell’s picture

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review

What world do we live in where drupal_set_message() is OK in OO code and trigger_error() isn't?

msonnabaum’s picture

Agree with catch.

There is no OOP argument for or against language level features. This is PHP, trigger_error() is a thing.

Crell’s picture

$_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*.

alexpott’s picture

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

  • if someone deploys a route to a production site that has this problem then they are doing it wrong
  • if someone releases a module of d.o that does this no will use it
  • the only time you should see this error is when you're developing a site

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.

dawehner’s picture

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

catch’s picture

Exception works for me - I'm fine with anything that results in a PHP error.

dawehner’s picture

FileSize
2.18 KB

Now we potentially have made all variants.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I think everyone is happy now.

Crell’s picture

Status: Reviewed & tested by the community » Needs work

Uh. 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.)

dawehner’s picture

FileSize
5.3 KB

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

Crell’s picture

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

dawehner’s picture

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

Crell’s picture

No, 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.

sun’s picture

Issue summary: View changes

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

sun’s picture

Status: Needs work » Needs review
FileSize
2.77 KB

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

dawehner’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
@@ -33,16 +33,15 @@ protected function alterRoutes(RouteCollection $collection) {
+    $is_valid = TRUE;
+    foreach ($collection->all() as $name => $route) {
...
+        $is_valid = FALSE;
...
-    return TRUE;
+    return $is_valid;

Mh, 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?

sun’s picture

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

sun’s picture

Sans return value — doesn't appear to be part of the RouteSubscriber::alter event API anyway.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/EventSubscriber/SpecialAttributesRouteSubscriber.php
@@ -33,16 +33,12 @@ protected function alterRoutes(RouteCollection $collection) {
+        trigger_error(sprintf('Route %s uses reserved variable names: %s', $name, $reserved), E_USER_WARNING);

+1 for improving the actual message.

[19:21] <sun> dawehner_: yup, phpunit converts errors into exceptions; hence, @expectedException \PHPUnit_Framework_Error_Warning

The last submitted patch, 35: 2051877-35.patch, failed testing.

Crell’s picture

Status: Reviewed & tested by the community » Needs work

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

dawehner’s picture

Logging 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:

        public function compile() {
          ...

            if (is_numeric($varName)) {
                throw new \DomainException(sprintf('Variable name "%s" cannot be numeric in route pattern "%s". Please use a different name.', $varName, $pattern));
            }
            if (in_array($varName, $variables)) {
                throw new \LogicException(sprintf('Route pattern "%s" cannot reference variable name "%s" more than once.', $pattern, $varName));
            }

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.

Crell’s picture

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

dawehner’s picture

We 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?

sun’s picture

Status: Needs work » Needs review

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

How is trigger_error() related to global state? It triggers the PHP error handler. You may register a custom error handler, but that is done via register_error_handler(). trigger_error() does not affect global state.

trigger_error() is to register_error_handler() like fopen() is to stream_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.

Crell’s picture

What 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. :-)

msonnabaum’s picture

Totally agreed with #50.

tim.plunkett’s picture

I also agree with #50.

dawehner’s picture

What sun said in #50!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Crell’s picture

Status: Reviewed & tested by the community » Needs work

As 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*?

tim.plunkett’s picture

@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?

sun’s picture

Status: Needs work » Needs review

@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?

sun’s picture

Just inject the log service and use that instead.

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

catch’s picture

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

catch’s picture

Status: Needs review » Reviewed & tested by the community

Putting #43 back to RTBC.

  • alexpott committed 9523119 on 8.x
    Issue #2051877 by dawehner, sun, tstoeckler: Log error when people use...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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