-
Notifications
You must be signed in to change notification settings - Fork 116
Skip bootstrap.php.cache/classes cache in debug mode #85
Conversation
$useDebugging = $environment === "dev"; | ||
} | ||
|
||
$isInteractiveDebugging = !$useDebugging ? false : isset( $_COOKIE['XDEBUG_SESSION'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the use of ternary operator a bit confusing.
I'd do :
$isInteractiveDebugging = false;
if ( $useDebugging && isset( $_COOKIE['XDEBUG_SESSION'] ) )
{
$isInteractiveDebugging = true;
$loader = require_once __DIR__ . '/../ezpublish/autoload.php';
}
else
{
$loader = require_once __DIR__ . '/../ezpublish/bootstrap.php.cache';
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
especially combined with negation in the predicate ;-)
on a more serious note: is this opening the door a bit to DOS attacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same concern, but it is activated only if you are using ez in dev mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't accept concerns, I accept use-cases.
More seriously, no, not that I can think of. Nothing changes unless you're either in the dev
env, or have the USE_DEBUGGING
env var set. Even if you open the dev env to a large audience for some reason, it won't enable remote debugging or anything, nor open up your ports. It just won't use bootrap.php.cache nor classes cache, and this will decrease performances.
But it still requires access to the dev env, which is something you don't have to fear DDOS attacks on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lolautruche your syntax makes sense, I'll try.
+1 besides my nitpick. Very useful. |
+1 |
@lolautruche is the logic okay with you now ? @gggeek was my answer satisfying to you ? |
Absolutely. It seems I had missed the 'it only works in dev mode' bit - exactly for the reason I was complaining about: I got the code wrong! ;-D |
yes, perfect! |
+0.1 There are some slightly crude things in here like hardcoding xdebug cookies(as also reflected in the task list), and also I know @lsmith77 has been involved in similar discussion on symfony-standard so would be nice to hear his take on this topic. |
i fought and lost symfony/symfony-standard#437 |
Well, after reading the battle report, it doesn't have to end the same way in this time & space :-) Those cookies are standard and well defined, and there is no security hit. From a SWOT perspective, no weaknesses and no threats, but clear Opportunities from a developer experience perspective. We must also take care to make it clear that the front controller is one of the files that SHOULD be modified. It's kinda of the whole point here... we can and should provide a really good, out of the box one, that will let people use our software decently before they start taking it apart. And about XDEBUG_COOKIE, it isn't even configurable on xdebug. |
No, point was more on the line that maybe we should just skip cache when in debug no matter which cookies are present or not, cleaner. You should consider benchmarking this btw, might have a direct impact on APC when includes are conditional instead. |
Good point |
I could agree with this alternative. There is no real point in using cached files during dev, they'll just require extra time to be built anyway. Is that fine by everybody ? Ditch the |
Are you guys fine with the latest version of the patch ? |
yes, only thing would be to benchmark it with a apache restart in between to wipe apc cache. |
Hmm, is something actually missing or not ? |
Well mentioned a few times; benchmark to make sure there is no negative impact on prod would be good. |
Benchmark the index in prod with & without the newly added checks, right ? |
Yes 👍 |
+10 : debugging is a bummer with classes cache |
+1 |
Let's merge this @bdunogier ! |
bootstrap.php.cache and classes cache can really get in the way with interactive debugging sessions. This changes the front controller so that when debugging, real classes are used instead of caches.
Skip bootstrap.php.cache/classes cache in debug mode
bootstrap.php.cache
and classes cache can really get in the way with interactive debugging sessions.This changes the front controller so that when debugging, if the \ XDEBUG_SESSION` cookie is set, the cache files above won't be loaded, so that the actual, original files can be stepped through.
Points to sort out