Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable Mini Profiler #6114

Merged
merged 2 commits into from Aug 16, 2019
Merged

Disable Mini Profiler #6114

merged 2 commits into from Aug 16, 2019

Conversation

elit0451
Copy link
Member

@elit0451 elit0451 commented Aug 13, 2019

Mini Profiler will be disabled when a solution is not in a debug mode, therefore Mini Profiler Startup Profiler won't be accessible.

Fixes: AB#2178

@Shazwazza Shazwazza merged commit cb7440b into v7/dev Aug 16, 2019
@Shazwazza Shazwazza deleted the v7/bug/2178-disable-mini-profiler branch August 16, 2019 05:37
@Shazwazza
Copy link
Contributor

@elit0451 great work!

@nul800sebastiaan I have merged and cherry picked to dev/7.15 branch ready to go for the next 7.15.3 patch.

@ronaldbarendse
Copy link
Contributor

This looks like a really ugly hack, as it removes ASP.NET MVC routes based on URL... Why not make sure the MiniProfiler doesn't add the routes to begin with (https://stackoverflow.com/questions/37681166/is-it-possible-to-disable-mini-profiler-handler)?

@ronaldbarendse
Copy link
Contributor

Or at least remove the routes based on the name: #5206 (comment)

@Shazwazza
Copy link
Contributor

@ronaldbarendse because the old mini profiler isn't that flexible. The code that creates the routes in the old profiler adds the routes on a static constructor so as soon as that class is referenced for any reason the routes are created. So yes it's a hack, but it's the only way to prevent this.

In 8 we don't need to do this and the new mini profiler is done in a nicer way without running logic on a static ctor.

@ronaldbarendse
Copy link
Contributor

I see, but removing the route based on the name would be better then on some configured URL prefix (that could therefore match more than the MiniProfiler route).

The route name is based on the prefix, so it's a quick rewrite: https://github.com/MiniProfiler/dotnet/blob/04f8ac465349a85a77d9fa4c6528fb75e63e157c/src/MiniProfiler/MiniProfilerHandler.cs#L59-L63.

@Shazwazza
Copy link
Contributor

Sure, if you have time to send a PR that would be superb

@Shazwazza
Copy link
Contributor

One thing to note is that code is referencing the newer version of MiniProfiler, you'll have to check back in history to when this was done in the static ctor and check what the route name might be there. It might not have changed but would need to be sure.

@ronaldbarendse
Copy link
Contributor

ronaldbarendse commented Sep 10, 2019

Just checked the decompiled code from MiniProfiler 2.1.0 (as the source isn't on GitHub) and the route is added the same way, but as it uses Insert, it's not using a named route 😬

The current fix is still too greedy, as it could still find the route based on the exact URL (instead of 'starts with'). The route URL is constructed as follows:

string prefix = MiniProfiler.Settings.RouteBasePath.Replace("~/", string.Empty).EnsureTrailingSlash();
string routeUrl = prefix + "{filename}";

The EnsureTrailingSlash() extension method should probably be changed to use the Umbraco built-in EnsureEndsWith(), but otherwise it's a quite simple change (I currently don't have the time the create a PR for it though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants