klein / klein.php

A fast & flexible router
MIT License
2.66k stars 290 forks source link

Fixed "Indirect modification of overloaded property" issue. #269

Closed Aristona closed 9 years ago

Aristona commented 9 years ago

Hi,

My Bugsnag was flooded with this error message from Klein. This is the line it is happening:

array_walk( (array) $req->widgets, function($val, $key) use ($app) {
    $app->widgetStatsDB->addImpression($val);
});

The whole error message is:

Indirect modification of overloaded property Klein\Request::$widgets has no effect

Not sure if this one creates more issues but that solved my issue. Perhabs it may be configured to do isset instead of passing by reference.

Can be a bug report too.

doctorallen commented 9 years ago

The ampersand is only supposed to be used to return a variable by reference, and should not be used in a method name. This causes a notice to be thrown. You can look at this stack overflow post which explains why this should not be done this way.

Rican7 commented 9 years ago

You're casting an "overloaded" (what PHP uses to refer to magically accessed) property, so PHP is attempting to warn you that the cast won't have a permanent stateful effect. If you'd like to have a casted version of that property, you'll want to first assign it to a local variable, like so:

$widgets = $req->widgets;

array_walk((array) $widgets, function($val, $key) use ($app) {
    $app->widgetStatsDB->addImpression($val);
});

If you'd rather not have a temporary variable, you can always skip the magic getter and access the param through the $request->param('widgets') method.

Changing the __get() method here will have considerable backwards compatibility breaking effects, so I can't allow for this change to be merged. Usually, as @drallen1 mentioned, implementing a __get() magic method with a reference return is not what you want to achieve here anyway.

Aristona commented 9 years ago

Thanks for the explanation. I didn't know I could reach it via param method. I knew what I tried to do was silly anyway. :)

Rican7 commented 9 years ago

No problem! Glad we could help. :)

Thanks @drallen1 for the help.