After #2445309: substr() expects parameter 1 to be string, object given in devel_silent I had a look more in depth at devel_silent(). The current implementation of this function not work properly because the path returned by Url::fromRoute('')->toString() (and the previous (implicit) behavior) has a leading slash and if your site runs in a subfolder the returned path is base_path + internal_path.
I think we need absolutely to fix this issue and modernize a bit devel_silent(). Patch attached

CommentFileSizeAuthor
#17 interdiff-12-17.txt670 byteswillzyx
#17 devel-devel_silent-2448391-17.patch11.56 KBwillzyx
#12 devel-devel_silent-2448391-12.patch11.55 KBwillzyx
#3 devel-devel_silent-2448391-3.patch1.33 KBwillzyx
#1 devel-devel_silent-2448391-1.patch1.32 KBwillzyx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willzyx’s picture

willzyx CreditAttribution: willzyx commented
FileSize
devel-devel_silent-2448391-1.patch1.32 KB

Status: Needs review » Needs work

The last submitted patch, 1: devel-devel_silent-2448391-1.patch, failed testing.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Needs review
FileSize
devel-devel_silent-2448391-3.patch1.33 KB

oops.. rerolled

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Priority: Normal » Major

Bumping to major. A lot of devel's functionalities not work properly due to this issue

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good. In addition, you can exclude system.db_update route and remove the clause about update.php. update.php is no longer its own file in D8. Feel free to commit after that change.

willzyx’s picture

willzyx CreditAttribution: willzyx commented

In addition, you can exclude system.db_update route and remove the clause about update.php. update.php is no longer its own file in D8

update.php seems to be a front controller after #2540416: Move update.php back to a front controller so i think we need that check

mermentau’s picture

mermentau CreditAttribution: mermentau as a volunteer commented

This is kinda urgent now due to changes in core that causes:

Fatal error: Call to undefined method Drupal\Core\Url::fromRoute() in C:\Users\John\Documents\My Webs\dev8\modules\devel\devel.module on line 239

The patch fixes that.

mermentau’s picture

mermentau CreditAttribution: mermentau as a volunteer commented

Disregard #7 as the error was caused by a corrupt download of Core.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman at Acquia commented
Status: Reviewed & tested by the community » Fixed

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Fixed » Needs work

Moshe I had not yet commited this because I think that is not the better solution. The current patch had some problems:

+++ b/devel.module
@@ -230,21 +230,30 @@ function devel_set_handler($handlers) {
+  $excluded_routes = array(
+    'file.ajax_progress',
+    'file.ajax_upload',
+    'image.style_private',
+    'system.batch_page.html',
+    'system.batch_page.json',
+    'system.files',
+    'system.private_file_download',
+  );

- It explicitly exclude some routes. From the viewpoint of maintainability it is not good.. we should constantly follow the core changes and keep our code aligned to HEAD.

- it not exclude non html responses, so we have not solved the problem. A lot of non html responses returned are still affected by this issue (a simple example: entity reference ajax call not work with this code). In devel_shutdown_real() we try to not break non-html pages relying on drupal_get_http_header that was removed by core.

- it miss test coverage

Proposed solution:
1) Enable devel only on html pages
2) Find a mantainable and simple way to do this
3) Find a way for explicitly exclude some route from devel output (eg add a _devel_silent requirement to routes) so developers are able to exclude theirs routes
4) add test coverage

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Needs review
FileSize
devel-devel_silent-2448391-12.patch11.55 KB

Here a patch that address proposed solution in #11 :
1) Enable devel only on html pages by checking the response type; No more explicitly exclude some routes
2) Is possible to manually disable devel adding the '_devel_silent' requirement to route definitions
3) Updated the doc block of devel_silent() to make a little bit clear how it works
4) Added test coverage

Status: Needs review » Needs work

The last submitted patch, 12: devel-devel_silent-2448391-12.patch, failed testing.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Needs review

GET /checkout/core/install.php returned 0 (0 bytes).

mmm.. retesting

willzyx queued 12: devel-devel_silent-2448391-12.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 12: devel-devel_silent-2448391-12.patch, failed testing.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Needs work » Needs review
FileSize
devel-devel_silent-2448391-17.patch11.56 KB
interdiff-12-17.txt670 bytes

Seems that sending a request with User-Agent: ApacheBench/1.0 header break the bot.. intresting
For now I commented that assertion

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks terrific - thanks. Sorry for my premature commit.

willzyx’s picture

willzyx CreditAttribution: willzyx commented
Status: Reviewed & tested by the community » Fixed

It's my fault. I would have to change the status of the issue and explain my reasons
Thanks for the patience :)

Committed and pushed to 8.x!

  • willzyx committed 3afa92d on 8.x-1.x 
    Issue #2448391 by willzyx: devel_silent() doesn't work properly

Status: Fixed » Closed (fixed)

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