Proposed commit message (D7)

Issue #1081266 by stefan.r, jeroen.b, mikeytown2, tsphethean, mfb, joseph.olstad, marcelovani, Fabianx: Avoid re-scanning module directory when a filename or a module is missing

Problem/Motivation

Drupal occurs a performance deficit when multiple filenames (ie. modules, themes) are missing or moved. The deficit occurs because we do a file scan perpetually to look for missing modules on certain pages, such as admin/config.

Proposed resolution

For D8: We remove the "missing file scan" that happens in drupal_get_filename() altogether and do various workarounds to deal with this scan in places where we haven't cached file locations yet, such as in the installer and in some tests (see #351). Instead of a file scan, we now do a trigger_error(), informing the developer/administrator about the missing file.

For D7: We cache any missing files in a static variable as well as in cache_bootstrap (for 24 hours, or until we visit the themes/modules overview page). We also do a trigger_error() as soon as we find a missing file in the drupal_get_filename() call.

In a followup issue we can check for this in hook_requirements() as well.

Remaining tasks

None.

User interface changes

None.

API changes

In D8, we remove the file scan from drupal_get_filename(), which also affects drupal_get_path(). The lack of this is only a "problem" in the installer, in tests and during module rebuild, which are not part of the API.

Original report by mfb

This patch provides a performance boost for sites that are missing modules (common on old sites which have been maintained for many years). In this case, drupal_system_listing() and file_scan_directory() will be called over and over looking for missing modules on certain pages, such as admin/config.

This patch adds an additional static variable to drupal_get_filename() to store an array of mask-directory combinations that have been scanned. Once one scan has been performed for a particular extension in a particular directory, there is no need to run the same scan later in the same request.

This information will be cached in 'drupal_get_filename:missing' in cache_bootstrap as well. Invalidation of this cache happens upon running cron, and upon visiting the themes/modules overview pages.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because significant performance slowdown
Issue priority Major because significant performance slowdown.
Prioritized changes The main goal of this issue is performance
Disruption N/A
CommentFileSizeAuthor
#611 interdiff.txt12.78 KBDavid_Rothstein
#611 1081266-611.patch39.79 KBDavid_Rothstein
#611 1081266-546-plus-new-tests-should-fail.patch36.87 KBDavid_Rothstein
#606 interdiff-546-606.txt3.24 KBDavid_Rothstein
#606 1081266-606.patch29.72 KBDavid_Rothstein
#603 interdiff-559-600.txt4.41 KBjeroen.b
#603 1081266-600.patch31.14 KBjeroen.b
#587 interdiff-582-587.txt4.45 KBjeroen.b
#587 interdiff-559-587.txt5.33 KBjeroen.b
#587 1081266-587.patch31.01 KBjeroen.b
#584 interdiff-559-582.txt3.95 KBjeroen.b
#583 1081266-582.patch30 KBjeroen.b
#559 interdiff-1081266-546-559.txt506 bytesjeroen.b
#559 1081266-559.patch29.12 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,781 pass(es). View
#546 interdiff.txt1013 bytesDavid_Rothstein
#546 1081266-546.patch28.57 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,791 pass(es). View
#532 interdiff.txt473 bytesDavid_Rothstein
#532 1081266-532.patch28.49 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,637 pass(es). View
#531 interdiff.txt34.18 KBDavid_Rothstein
#531 1081266-531.patch28.54 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 41,625 pass(es). View
#527 interdiff-524-527.txt0 bytesstefan.r
#527 1081266-527.patch23.61 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 41,606 pass(es), 0 fail(s), and 3 exception(s). View
#524 1081266-524.patch23.3 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 41,609 pass(es), 2 fail(s), and 0 exception(s). View
#524 interdiff-518-524.txt11.85 KBstefan.r
#518 1081266-518.patch22.91 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,619 pass(es). View
#518 interdiff-511-518.txt5.92 KBstefan.r
#512 interdiff-499-511.txt762 bytesstefan.r
#512 1081266-511.patch22.8 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,497 pass(es). View
#499 interdiff-498-499.txt1.43 KBstefan.r
#499 1081266-499.patch22.64 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,484 pass(es). View
#498 1081266-498.patch22.65 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,486 pass(es). View
#498 interdiff-496-498.txt5.9 KBstefan.r
#496 1081266-496.patch20.94 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,483 pass(es). View
#496 interdiff-492-496.txt887 bytesstefan.r
#492 1081266-492.patch20.94 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
#492 interdiff-491-492.txt1.03 KBstefan.r
#491 1081266-491.patch20.94 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,491 pass(es). View
#491 interdiff-483-491.txt3.08 KBstefan.r
#483 interdiff-481-483.txt3.32 KBstefan.r
#483 1081266-483.patch19.92 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
#480 interdiff-480-481.txt2.03 KBstefan.r
#480 1081266-481.patch19.73 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
#479 1081266-480.patch19.78 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,480 pass(es). View
#479 interdiff-479-480.txt9.83 KBstefan.r
#478 interdiff-478-479.txt4.38 KBstefan.r
#478 1081266-479.patch19.09 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,482 pass(es). View
#475 1081266-478.patch19.9 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
#475 interdiff-456-478.txt21.68 KBstefan.r
#456 1081266-456.patch15.89 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,487 pass(es). View
#456 interdiff-453-456.txt786 bytesstefan.r
#453 interdiff-452-453.txt2.23 KBstefan.r
#453 1081266-453.patch15.79 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,501 pass(es). View
#452 interdiff-449-452.txt9.34 KBstefan.r
#452 1081266-452.patch15.44 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,492 pass(es). View
#449 interdiff-437-449.txt5.7 KBstefan.r
#449 interdiff-445-449.txt3.06 KBstefan.r
#449 1081266-449.patch12.27 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,483 pass(es). View
#445 1081266-445.patch12.09 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 41,493 pass(es), 1 fail(s), and 0 exception(s). View
#445 interdiff-444-445.txt2 KBstefan.r
#444 interdiff-437-444.txt4.04 KBstefan.r
#444 1081266-444.patch11.59 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 41,486 pass(es), 1 fail(s), and 0 exception(s). View
#437 interdiff-425-437.txt9.13 KBstefan.r
#437 1081266-437.patch8.24 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,491 pass(es). View
#436 interdiff-425-436.txt7.34 KBstefan.r
#436 1081266-436.patch8.12 KBstefan.r
PASSED: [[SimpleTest]]: [MySQL] 41,477 pass(es). View
#425 1081266-425.patch7.52 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
#425 interdiff-1081266-419-425.txt2.15 KBjeroen.b
#419 1081266-419.patch7.5 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,487 pass(es). View
#419 interdiff-1081266-415-419.txt716 bytesjeroen.b
#415 1081266-415.patch7.36 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,489 pass(es). View
#415 interdiff-1081266-407-415.txt2.56 KBjeroen.b
#408 interdiff-1081266-373-407.txt8.48 KBjeroen.b
#407 1081266-407.patch7.27 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,485 pass(es). View
#407 interdiff-1081266-402-407.txt1.83 KBjeroen.b
#405 interdiff-1081266-373-403.txt8.36 KBjeroen.b
#403 1081266-402.patch7.16 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,489 pass(es). View
#403 interdiff-1081266-399-402.txt1.16 KBjeroen.b
#399 1081266-399.patch5.91 KBjeroen.b
FAILED: [[SimpleTest]]: [MySQL] 41,492 pass(es), 0 fail(s), and 6 exception(s). View
#399 interdiff-1081266-397-399.txt865 bytesjeroen.b
#397 interdiff-1081266-391-397.txt473 bytesjeroen.b
#397 1081266-397.patch5.86 KBjeroen.b
FAILED: [[SimpleTest]]: [MySQL] 41,489 pass(es), 0 fail(s), and 7 exception(s). View
#395 1081266-395-do-not-test.patch5.42 KBjeroen.b
#391 1081266-391.patch5.35 KBjeroen.b
FAILED: [[SimpleTest]]: [MySQL] 41,488 pass(es), 0 fail(s), and 301 exception(s). View
#389 interdiff-1081266-379-389.txt3.14 KBjeroen.b
#389 1081266-389.patch5.44 KBjeroen.b
FAILED: [[SimpleTest]]: [MySQL] 41,486 pass(es), 0 fail(s), and 302 exception(s). View
#387 1081266-387.patch5.52 KBjeroen.b
FAILED: [[SimpleTest]]: [MySQL] 41,489 pass(es), 0 fail(s), and 305 exception(s). View
#385 1081266-385.patch5.52 KBjeroen.b
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/bootstrap.test. View
#385 interdiff-1081266-381-385.txt2.24 KBjeroen.b
#383 interdiff-1081266-379-381.txt940 bytesjeroen.b
#381 interdiff-1081266-379-381.patch940 bytesjeroen.b
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1081266-379-381.patch. Unable to apply patch. See the log in the details link for more information. View
#381 1081266-381.patch5.24 KBjeroen.b
FAILED: [[SimpleTest]]: [MySQL] 41,483 pass(es), 2 fail(s), and 303 exception(s). View
#379 interdiff-375-379.txt2.71 KBstefan.r
#379 1081266-379.patch5.24 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 40,000 pass(es), 224 fail(s), and 370 exception(s). View
#376 interdiff-374-375.txt6.57 KBstefan.r
#375 interdiff-374-375.txt0 bytesstefan.r
#375 drupal-1081266-375-avoid-rescanning-missing-module-D7.patch5.22 KBstefan.r
FAILED: [[SimpleTest]]: [MySQL] 39,998 pass(es), 224 fail(s), and 368 exception(s). View
#374 drupal-1081266-373-avoid-rescanning-missing-module-D7.patch3.31 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,484 pass(es). View
#367 interdiff-360-367.txt3.13 KBstefan.r
#367 1081266-367.patch36.97 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,282 pass(es). View
#362 interdiff-289-360.txt34.24 KBstefan.r
#361 interdiff-358-360.txt2.16 KBstefan.r
#360 1081266-360.patch36.97 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,279 pass(es). View
#360 1081266-360.patch36.97 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,265 pass(es). View
#358 interdiff-356-358.txt806 bytesstefan.r
#358 1081266-358.patch36.43 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,241 pass(es), 0 fail(s), and 10 exception(s). View
#357 1081266-356.patch36.41 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,252 pass(es). View
#357 interdiff-354-356.txt652 bytesstefan.r
#355 interdiff-289-354.txt33.54 KBstefan.r
#355 interdiff-352-354.txt10.07 KBstefan.r
#355 1081266-354.patch36.45 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 26,761 pass(es), 982 fail(s), and 1,048 exception(s). View
#353 interdiff-350-352.txt8.08 KBstefan.r
#352 1081266-352.patch35.14 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,177 pass(es). View
#352 1081266-352.patch35.14 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,170 pass(es). View
#350 1081266-350.patch33.3 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,189 pass(es). View
#350 interdiff-347-350.txt3.98 KBstefan.r
#347 1081266-347.patch32.53 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,930 pass(es). View
#347 interdiff-342-347.txt3.76 KBstefan.r
#344 interdiff-335-342.txt4.29 KBstefan.r
#344 1081266-342.patch32.67 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,920 pass(es). View
#335 interdiff-331-335.txt6.09 KBstefan.r
#335 1081266-335.patch32.27 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,923 pass(es). View
#331 1081266-331.patch27.79 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,896 pass(es). View
#331 interdiff-312-331.txt22.99 KBstefan.r
#329 interdiff-325-329.txt6.04 KBstefan.r
#329 1081266-329.patch26.92 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,889 pass(es), 3 fail(s), and 0 exception(s). View
#325 1081266-325.patch20.81 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#323 interdiff-314-323.txt10.04 KBstefan.r
#323 1081266-323.patch19.25 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 53,495 pass(es), 7,561 fail(s), and 7,642 exception(s). View
#321 interdiff-314-321.txt9.75 KBstefan.r
#321 1081266-321.patch18.96 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#315 interdiff-312-314.txt3.79 KBstefan.r
#315 1081266-314.patch9.34 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#312 interdiff-289-312.txt2.7 KBstefan.r
#312 1081266-312.patch13.3 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#289 1081266-289.patch15.11 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,862 pass(es). View
#289 interdiff-266-289.txt4.06 KBstefan.r
#287 1081266-287.patch15.23 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,759 pass(es), 0 fail(s), and 4 exception(s). View
#287 interdiff-283-287.txt1.24 KBstefan.r
#284 1081266-283.patch13.77 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,767 pass(es), 0 fail(s), and 11 exception(s). View
#284 interdiff-266-283.txt2.91 KBstefan.r
#282 1081266-282.patch13.77 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/ConfigInstaller.php. View
#282 interdiff-281-282.txt1.48 KBstefan.r
#281 1081266-281.patch13.47 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,705 pass(es), 0 fail(s), and 719 exception(s). View
#281 interdiff-278-281.txt1.23 KBstefan.r
#279 1081266-278.patch12.13 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,726 pass(es), 0 fail(s), and 730 exception(s). View
#279 interdiff-277-278.txt363 bytesstefan.r
#277 interdiff-274-277.txt907 bytesstefan.r
#277 1081266-277.patch12.17 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/ExtensionInstallStorage.php. View
#274 1081266-274.patch11.16 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,714 pass(es), 0 fail(s), and 799 exception(s). View
#274 interdiff-266-274.txt551 bytesstefan.r
#266 1081266-266.patch11.5 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,695 pass(es). View
#266 interdiff-260-266.txt1.16 KBstefan.r
#261 1081266-260.patch11.95 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,428 pass(es). View
#261 interdiff-254-260.txt1.51 KBstefan.r
#258 interdiff-223-254.txt9.3 KBstefan.r
#254 1081266-254.patch11.94 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,982 pass(es). View
#252 interdiff-233-248.txt9.05 KBstefan.r
#248 interdiff.txt3.56 KBstefan.r
#248 1081266-248.patch11.91 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,981 pass(es). View
#245 1081266-244.patch11.88 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,717 pass(es), 201 fail(s), and 81 exception(s). View
#243 interdiff.txt3.56 KBstefan.r
#243 1081266-243.patch11.8 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Controller/SystemController.php. View
#241 interdiff.txt7.14 KBstefan.r
#241 1081266-241.patch9.84 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Tests/Bootstrap/GetFilenameUnitTest.php. View
#233 drupal-1081266-233-avoid-rescanning-missing-module-D8.patch6.96 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,964 pass(es), 0 fail(s), and 6 exception(s). View
#233 interdiff.txt684 bytesstefan.r
#223 interdiff.txt1.58 KBstefan.r
#223 drupal-1081266-223--avoid-rescanning-missing-module-D8.patch7.01 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,919 pass(es). View
#221 drupal-1081266-219-avoid-rescanning-missing-module-D8.patch6.83 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,917 pass(es). View
#221 interdiff.txt5.64 KBstefan.r
#202 drupal-1081266-202-avoid-rescanning-missing-module-D8.patch4.42 KBjoseph.olstad
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,705 pass(es). View
#189 drupal-1081266-189-avoid-rescanning-missing-module-D7.patch3.31 KBjeroen.b
PASSED: [[SimpleTest]]: [MySQL] 41,135 pass(es). View
#186 drupal-1081266-186-avoid-rescanning-missing-module-D8.patch5.44 KBjeroen.b
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,309 pass(es). View
#181 includes_menu_inc.png225.58 KBjoelpittet
#173 bootstrap_inc_-.png198.21 KBjoelpittet
#171 drupal-1081266-171-drupal_get_filename-extra-debug-do-not-test-D7.patch1.87 KBjeroen.b
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-1081266-171-drupal_get_filename-extra-debug-do-not-test-D7.patch. Unable to apply patch. See the log in the details link for more information. View
#162 drupal-1081266-162-avoid-rescanning-missing-module-D8.patch3.78 KBjeroen.b
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,136 pass(es). View
#159 drupal-1081266-159-avoid-rescanning-missing-module-D8.patch3.76 KBmikeytown2
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,539 pass(es). View
#157 drupal-1081266-157-avoid-rescanning-missing-module-D8.patch3.76 KBmikeytown2
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/includes/bootstrap.inc. View
#155 drupal-1081266-155-avoid-rescanning-missing-module-D8.patch3.75 KBmikeytown2
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/includes/bootstrap.inc. View
#150 drupal-1081266-137-avoid-rescanning-missing-module-D8.patch5.39 KBmarcelovani
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,441 pass(es). View
#138 drupal-1081266-138-drupal_get_filename-D7.patch1.8 KBjeroen.b
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-1081266-138-drupal_get_filename-D7.patch. Unable to apply patch. See the log in the details link for more information. View
#137 drupal-1081266-137-avoid-rescanning-missing-module-D8.patch5.39 KBjeroen.b
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,039 pass(es). View
#132 drupal-1081266-132-avoid-rescanning-missing-module-D8.patch5.54 KBmikeytown2
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,037 pass(es). View
#127 drupal-1081266-127-avoid-rescanning-missing-module-D8.patch5.32 KBmikeytown2
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,994 pass(es). View
#124 drupal-1081266-124-avoid-rescanning-missing-module-D8.patch4.12 KBmikeytown2
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,957 pass(es), 1 fail(s), and 0 exception(s). View
#120 avoid_rescanning_drupal_get_filename-1081266-120.patch2.89 KBjeroen.b
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,956 pass(es), 2 fail(s), and 1 exception(s). View
#102 drupal-1081266-102-drupal_get_filename-D7.patch1.75 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 39,617 pass(es). View
#95 drupal_get_filename_cache-1081266-95.patch7.09 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_get_filename_cache-1081266-95.patch. Unable to apply patch. See the log in the details link for more information. View
#93 drupal_get_filename_cache-1081266-93.patch7.09 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_get_filename_cache-1081266-93.patch. Unable to apply patch. See the log in the details link for more information. View
#80 drupal_get_filename_cache-1081266-80.patch7.24 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] 40,161 pass(es), 97 fail(s), and 82 exception(s). View
#74 drupal_get_filename_cache-1081266-74.patch7.09 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] 40,038 pass(es), 97 fail(s), and 82 exception(s). View
#69 drupal_get_filename_cache-1081266-69.patch7.1 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#64 drupal-1081266-64-drupal_get_filename-D8.patch1.78 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 36,983 pass(es). View
#62 drupal-1081266-62-drupal_get_filename-D8.patch1.79 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 36,999 pass(es), 0 fail(s), and 130 exception(s). View
#60 drupal-1081266-60-drupal_get_filename-D8.patch1.77 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#58 drupal-1081266-58-drupal_get_filename-D7.patch1.75 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 39,062 pass(es). View
#30 image1-7.8-default.png54.12 KBKars-T
#30 Image2-7.8-patch_from_1081266.png43.2 KBKars-T
#30 Image3-7.8_patch_from_1081266_and_733308.png35.19 KBKars-T
#30 Image4-7.8_patch_from_733308.png40.7 KBKars-T
2011-10-01_025602.png7.66 KBxandeadx
#9 1081266-drupal_get_filename.patch1.99 KBmfb
PASSED: [[SimpleTest]]: [MySQL] 29,486 pass(es). View
#4 1081266-drupal_get_filename-D7.patch2.37 KBmfb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1081266-drupal_get_filename-D7.patch. Unable to apply patch. See the log in the details link for more information. View
drupal_get_filename.patch1.99 KBmfb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_get_filename_4.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

catch’s picture

Assigned: catch » Unassigned

Didn't do a thorough review but I think the changes here are OK as well. It's verbose but the trade-off vs supporting this behaviour on all requests is worth it - and the @todos mean we have a decent chance to clean things up when the listing/info issue goes in.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Then lets RTBC this.

stefan.r’s picture

FileSize
36.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,282 pass(es). View
3.13 KB

This adds the spelling corrections I had mentioned earlier

Fabianx’s picture

+++ b/core/modules/system/src/Tests/Extension/ModuleHandlerTest.php
@@ -201,7 +201,7 @@
-    drupal_get_filename('profile', 'minimal', 'core/profiles/' . $profile . '/' . $profile . '.info.yml');
+    drupal_get_filename('profile', $profile, 'core/profiles/' . $profile . '/' . $profile . '.info.yml');

This change was what you wanted?

And the comment above should still speak of minimal?

stefan.r’s picture

Yes, that was merely for code readability. The profile is hardcoded to minimal a line above this all, if it gets changed, people can update the comment.

Tests will be red if this is wrong :)

stefan.r’s picture

Issue summary: View changes
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed b5f7ccc on 8.0.x
    Issue #1081266 by stefan.r, mikeytown2, jeroen.b, tsphethean, mfb,...
David_Rothstein’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
Issue tags: +needs backport to D7

Pretty sure there was an intention to backport at least some part of this (whether the trigger_error or the trigger_error + cache or something else).

jeroen.b’s picture

FileSize
3.31 KB
PASSED: [[SimpleTest]]: [MySQL] 41,484 pass(es). View

Drupal 7 patch was already finished (without the trigger_error).
If we are not going to do such an extensive rewrite of drupal_get_filename in D7 we can use that as a start.

stefan.r’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.22 KB
FAILED: [[SimpleTest]]: [MySQL] 39,998 pass(es), 224 fail(s), and 368 exception(s). View
0 bytes

Thanks @jeroen.b for the D7 patch and thanks again @Fabianx and @catch for the helpful reviews.

The trigger_error in the D8 patch made a whole bunch of tests fail, let's see how this one does.

stefan.r’s picture

FileSize
6.57 KB

Status: Needs review » Needs work

The last submitted patch, 375: drupal-1081266-375-avoid-rescanning-missing-module-D7.patch, failed testing.

marcelovani’s picture

Nice work with the D8 patch.
Now, back to D7, when I go to the modules page, I get this error:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE (cid = 'drupal_get_filename:missing')' at line 1: DELETE FROM {} WHERE (cid = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => drupal_get_filename:missing ) in cache_clear_all() (line 167 of /Users/marcelo/vm/vagrant/repos/drupal/includes/cache.inc).

ps: I am testing with DB cache.

It looks like, when clearing the cache, providing a cid, the parameter bin is required. See http://cgit.drupalcode.org/drupal/tree/includes/cache.inc?h=7.x#n150

in this case, you would have to change it to cache_clear_all('drupal_get_filename:missing', 'cache'); in the two functions.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
5.24 KB
FAILED: [[SimpleTest]]: [MySQL] 40,000 pass(es), 224 fail(s), and 370 exception(s). View
2.71 KB

Status: Needs review » Needs work

The last submitted patch, 379: 1081266-379.patch, failed testing.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
5.24 KB
FAILED: [[SimpleTest]]: [MySQL] 41,483 pass(es), 2 fail(s), and 303 exception(s). View
940 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1081266-379-381.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed the cache_clear_all

Status: Needs review » Needs work

The last submitted patch, 381: interdiff-1081266-379-381.patch, failed testing.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
940 bytes

Interdiff in a txt now to prevent testing.

The last submitted patch, 381: 1081266-381.patch, failed testing.

jeroen.b’s picture

FileSize
2.24 KB
5.52 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/bootstrap.test. View

Trying to fix the fails.

Status: Needs review » Needs work

The last submitted patch, 385: 1081266-385.patch, failed testing.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
5.52 KB
FAILED: [[SimpleTest]]: [MySQL] 41,489 pass(es), 0 fail(s), and 305 exception(s). View

Whoops, that was stupid.

Status: Needs review » Needs work

The last submitted patch, 387: 1081266-387.patch, failed testing.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
5.44 KB
FAILED: [[SimpleTest]]: [MySQL] 41,486 pass(es), 0 fail(s), and 302 exception(s). View
3.14 KB

I think this is a little cleaner approach without cache or statics in the error handler.
Should have no fails. I wonder where all the "The following module is missing from the file system: default." are coming from though.

Also included an interdiff with #379.

Status: Needs review » Needs work

The last submitted patch, 389: 1081266-389.patch, failed testing.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
FAILED: [[SimpleTest]]: [MySQL] 41,488 pass(es), 0 fail(s), and 301 exception(s). View

Rewrote one comment.
Fixed one of our own tests.

The "default" module updates are all in upgrade tests. For some reason there is a "default" record in the system table for those upgrade tests.
The "simpletest_missing_module" is from a test from simpletest to actually test for a missing module.
Any suggestions to solve that?

Status: Needs review » Needs work

The last submitted patch, 391: 1081266-391.patch, failed testing.

marcelovani’s picture

I don't see the "default" but I do see this
"The following module is missing from the file system: ."

marcelovani’s picture

That is related to $name being passed as empty by other contrib/core modules
We already spoke about this here https://www.drupal.org/node/2383823

jeroen.b’s picture

Can you try to figure out what module is doing that and open an issue for that project?
Current status of the patch is that it only fails in tests because:

  1. There is 1 test that actually wants a module to be missing
  2. For some reason the upgrade db seeds contain a "default" module in the system table

Point 1 I'm not really sure how to fix that without removing the test.
Point 2 I'm not really confident that I know why the db seed contains a "default" module.

Also attached a patch in which it's easier to find the causing module for the trigger_error because it adds an debug_backtrace to the trigger_error.

jeroen.b’s picture

Dicovered something, this seems really wrong, this is from the system table in the upgrade DB seed:

[filename] => profiles/default/default.profile
        [name] => default
        [type] => module
        [owner] => 
        [status] => 0
        [bootstrap] => 0
        [schema_version] => 0
        [weight] => 1000
        [info] =>
    a:6:{s:12:"dependencies";a:0:{}s:11:"description";s:0:"";s:7:"package";s:5:"Other";s:7:"version";N;s:3:"php";s:5:"5.2.4";s:5:"files";a:0:{}}
jeroen.b’s picture

Status: Needs work » Needs review
FileSize
5.86 KB
FAILED: [[SimpleTest]]: [MySQL] 41,489 pass(es), 0 fail(s), and 7 exception(s). View
473 bytes

I think I have a fix for the missing "default" module. In Drupal 7 it has been renamed to "standard".
This gets updated in a hook_update_n() of the system module but obviously this code runs before running the updates.

Status: Needs review » Needs work

The last submitted patch, 397: 1081266-397.patch, failed testing.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
865 bytes
5.91 KB
FAILED: [[SimpleTest]]: [MySQL] 41,492 pass(es), 0 fail(s), and 6 exception(s). View

This should fix all the upgrade path tests. Only one thats left now are the lookups for simpletest_missing_module.

Status: Needs review » Needs work

The last submitted patch, 399: 1081266-399.patch, failed testing.

jeroen.b’s picture

Indeed, only the simpletest_missing_module left now. It's caused by modules/simpletest/simpletest.test:

/**
 * Test required modules for tests.
 */
class SimpleTestMissingDependentModuleUnitTest extends DrupalUnitTestCase {
  public static function getInfo() {
    return array(
      'name' => 'Testing dependent module test',
      'description' => 'This test should not load since it requires a module that is not found.',
      'group' => 'SimpleTest',
      'dependencies' => array('simpletest_missing_module'),
    );
  }

  /**
   * Ensure that this test will not be loaded despite its dependency.
   */
  function testFail() {
    $this->fail(t('Running test with missing required module.'));
  }
}
joshtaylor’s picture

Running just this, and no contrib modules, trying to do a test run on any core module gives me the following:

PHP Warning:  The following module is missing from the file system: standard in core/includes/bootstrap.inc on line 254
PHP Stack trace:
PHP   1. {main}() core/scripts/run-tests.sh:0
PHP   2. Drupal\Core\DrupalKernel->prepareLegacyRequest() core/scripts/run-tests.sh:39
PHP   3. Drupal\Core\DrupalKernel->preHandle() core/lib/Drupal/Core/DrupalKernel.php:611
PHP   4. Drupal\Core\Config\ConfigFactory->get() core/lib/Drupal/Core/DrupalKernel.php:510
PHP   5. Drupal\Core\Config\ConfigFactory->doGet() core/lib/Drupal/Core/Config/ConfigFactory.php:94
PHP   6. Drupal\Core\Config\ConfigFactory->doLoadMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:109
PHP   7. Drupal\Core\Config\FileStorage->readMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:166
PHP   8. Drupal\Core\Config\FileStorage->read() core/lib/Drupal/Core/Config/FileStorage.php:118
PHP   9. Drupal\Core\Config\InstallStorage->exists() core/lib/Drupal/Core/Config/FileStorage.php:96
PHP  10. Drupal\Core\Config\InstallStorage->getAllFolders() core/lib/Drupal/Core/Config/InstallStorage.php:103
PHP  11. Drupal\Core\Extension\ExtensionDiscovery->scan() core/lib/Drupal/Core/Config/InstallStorage.php:173
PHP  12. Drupal\Core\Extension\ExtensionDiscovery->setProfileDirectoriesFromSettings() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:140
PHP  13. drupal_get_path() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:219
PHP  14. drupal_get_filename() core/includes/bootstrap.inc:277
PHP  15. trigger_error() core/includes/bootstrap.inc:254

Warning: The following module is missing from the file system: standard in core/includes/bootstrap.inc on line 254

Call Stack:
    0.0012     443952   1. {main}() core/scripts/run-tests.sh:0
    0.0087    2021584   2. Drupal\Core\DrupalKernel->prepareLegacyRequest() core/scripts/run-tests.sh:39
    0.3772   14023512   3. Drupal\Core\DrupalKernel->preHandle() core/lib/Drupal/Core/DrupalKernel.php:611
    0.3863   15337176   4. Drupal\Core\Config\ConfigFactory->get() core/lib/Drupal/Core/DrupalKernel.php:510
    0.3863   15337352   5. Drupal\Core\Config\ConfigFactory->doGet() core/lib/Drupal/Core/Config/ConfigFactory.php:94
    0.3863   15337976   6. Drupal\Core\Config\ConfigFactory->doLoadMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:109
    0.3863   15338512   7. Drupal\Core\Config\FileStorage->readMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:166
    0.3863   15339080   8. Drupal\Core\Config\FileStorage->read() core/lib/Drupal/Core/Config/FileStorage.php:118
    0.3863   15339120   9. Drupal\Core\Config\InstallStorage->exists() core/lib/Drupal/Core/Config/FileStorage.php:96
    0.3864   15339392  10. Drupal\Core\Config\InstallStorage->getAllFolders() core/lib/Drupal/Core/Config/InstallStorage.php:103
    0.3973   15364360  11. Drupal\Core\Extension\ExtensionDiscovery->scan() core/lib/Drupal/Core/Config/InstallStorage.php:173
    0.3973   15364544  12. Drupal\Core\Extension\ExtensionDiscovery->setProfileDirectoriesFromSettings() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:140
    0.3973   15364728  13. drupal_get_path() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:219
    0.3973   15364728  14. drupal_get_filename() core/includes/bootstrap.inc:277
    0.3983   15483184  15. trigger_error() core/includes/bootstrap.inc:254

PHP Warning:  strpos(): Empty needle in core/lib/Drupal/Core/Extension/ExtensionDiscovery.php on line 270
PHP Stack trace:
PHP   1. {main}() core/scripts/run-tests.sh:0
PHP   2. Drupal\Core\DrupalKernel->prepareLegacyRequest() core/scripts/run-tests.sh:39
PHP   3. Drupal\Core\DrupalKernel->preHandle() core/lib/Drupal/Core/DrupalKernel.php:611
PHP   4. Drupal\Core\Config\ConfigFactory->get() core/lib/Drupal/Core/DrupalKernel.php:510
PHP   5. Drupal\Core\Config\ConfigFactory->doGet() core/lib/Drupal/Core/Config/ConfigFactory.php:94
PHP   6. Drupal\Core\Config\ConfigFactory->doLoadMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:109
PHP   7. Drupal\Core\Config\FileStorage->readMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:166
PHP   8. Drupal\Core\Config\FileStorage->read() core/lib/Drupal/Core/Config/FileStorage.php:118
PHP   9. Drupal\Core\Config\InstallStorage->exists() core/lib/Drupal/Core/Config/FileStorage.php:96
PHP  10. Drupal\Core\Config\InstallStorage->getAllFolders() core/lib/Drupal/Core/Config/InstallStorage.php:103
PHP  11. Drupal\Core\Extension\ExtensionDiscovery->scan() core/lib/Drupal/Core/Config/InstallStorage.php:173
PHP  12. Drupal\Core\Extension\ExtensionDiscovery->filterByProfileDirectories() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:190
PHP  13. array_filter() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:277
PHP  14. Drupal\Core\Extension\ExtensionDiscovery->Drupal\Core\Extension\{closure}() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:277
PHP  15. strpos() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:270

Warning: strpos(): Empty needle in core/lib/Drupal/Core/Extension/ExtensionDiscovery.php on line 270

Call Stack:
    0.0012     443952   1. {main}() core/scripts/run-tests.sh:0
    0.0087    2021584   2. Drupal\Core\DrupalKernel->prepareLegacyRequest() core/scripts/run-tests.sh:39
    0.3772   14023512   3. Drupal\Core\DrupalKernel->preHandle() core/lib/Drupal/Core/DrupalKernel.php:611
    0.3863   15337176   4. Drupal\Core\Config\ConfigFactory->get() core/lib/Drupal/Core/DrupalKernel.php:510
    0.3863   15337352   5. Drupal\Core\Config\ConfigFactory->doGet() core/lib/Drupal/Core/Config/ConfigFactory.php:94
    0.3863   15337976   6. Drupal\Core\Config\ConfigFactory->doLoadMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:109
    0.3863   15338512   7. Drupal\Core\Config\FileStorage->readMultiple() core/lib/Drupal/Core/Config/ConfigFactory.php:166
    0.3863   15339080   8. Drupal\Core\Config\FileStorage->read() core/lib/Drupal/Core/Config/FileStorage.php:118
    0.3863   15339120   9. Drupal\Core\Config\InstallStorage->exists() core/lib/Drupal/Core/Config/FileStorage.php:96
    0.3864   15339392  10. Drupal\Core\Config\InstallStorage->getAllFolders() core/lib/Drupal/Core/Config/InstallStorage.php:103
    0.3973   15364360  11. Drupal\Core\Extension\ExtensionDiscovery->scan() core/lib/Drupal/Core/Config/InstallStorage.php:173
    0.3987   15494016  12. Drupal\Core\Extension\ExtensionDiscovery->filterByProfileDirectories() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:190
    0.3987   15494424  13. array_filter() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:277
    0.3988   15504440  14. Drupal\Core\Extension\ExtensionDiscovery->Drupal\Core\Extension\{closure}() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:277
    0.3988   15504720  15. strpos() core/lib/Drupal/Core/Extension/ExtensionDiscovery.php:270

I'm on Ubuntu, I also get this on Travis with PHP 5.4/5.5/5.6 - https://travis-ci.org/joshuataylor/commerce/builds/61874609

jeroen.b’s picture

FileSize
1.16 KB
7.16 KB
PASSED: [[SimpleTest]]: [MySQL] 41,489 pass(es). View

@joshtaylor D8 has already been committed, maybe it's better to open a new issue for that.

I went for the same method as the bootstrap tests. All tests should be green now. Not sure if it's the way to go though.

jeroen.b’s picture

Status: Needs work » Needs review
jeroen.b’s picture

For reference, interdiff with #373.

joshtaylor’s picture

Created a followup issue https://www.drupal.org/node/2486083

jeroen.b’s picture

FileSize
1.83 KB
7.27 KB
PASSED: [[SimpleTest]]: [MySQL] 41,485 pass(es). View

Some minor tweaks and found a mistake with restoring the error handler.

jeroen.b’s picture

Updated interdiff with #373.

fgm’s picture

Status: Needs review » Needs work

New side-effect of this patch: run-scripts is broken for some configurations #2486083: The following module is missing from the file system: standard

fgm’s picture

Version: 7.x-dev » 8.0.x-dev
jeroen.b’s picture

Why did you put it back to 8.0.x-dev?
Can you please keep patches/discussions about the new errors in the new issue?

jeroen.b’s picture

Status: Needs work » Needs review

Other issue has a patch and is RTBC, so let's keep this at 7.x-dev.

jeroen.b’s picture

Version: 8.0.x-dev » 7.x-dev
stefan.r’s picture

Status: Needs review » Needs work

Well @catch and @Fabianx had already mentioned the caching+trigger_error could work for D7, and #407 is essentially equal to the previously RTBC'ed D8 patch in #289. It's green too, so I guess we're close here?

  1. +++ b/includes/update.inc
    @@ -795,6 +795,12 @@ function update_fix_d7_requirements() {
    +  // Default profile has been renamed to Standard in D7.
    +  if ($profile == 'default') {
    +    $profile = 'standard';
    +    variable_set('install_profile', $profile);
    +  }
    

    This could use better documentation -- why are we changing the profile from default to standard. Are there any possible secondary effects here?

  2. +++ b/modules/simpletest/simpletest.module
    @@ -370,11 +370,27 @@ function simpletest_test_get_all() {
    +            // Searching for an item that does not exist triggers an PHP error.
    +            // Set a custom error handler so we can ignore the file not found error.
    

    80 cols

  3. +++ b/modules/simpletest/simpletest.module
    @@ -370,11 +370,27 @@ function simpletest_test_get_all() {
    +            set_error_handler(function($error_level, $message, $filename, $line, $context) {
    
    +++ b/modules/simpletest/simpletest.module
    @@ -370,11 +370,27 @@ function simpletest_test_get_all() {
    +                // Restore the original error handler.
    +                restore_error_handler();
                     continue 2;
                   }
                 }
    +
    +            // Restore the original error handler.
    +            restore_error_handler();
    

    Maybe this should merely wrap the drupal_get_filename call, I don't see that we need to put it around the other statements and having restore_error_handler there twice looks confusing.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
2.56 KB
7.36 KB
PASSED: [[SimpleTest]]: [MySQL] 41,489 pass(es). View

Addresses your comments.
I think the context of update_fix_d7_install_profile() and the "@see system_update_7049()" comment are enough now, what do you think?

mikeytown2’s picture

#415 looks good! Hopefully not to many people have the error_level set to 1 or 2 on a production site with a missing module.

jeroen.b’s picture

Yeah this should definitely be mentioned in the release notes. Possibly with a link to a documentation page with the following structure:

<h2>How to fix "The following module is missing from the file system"?</h2>


<h3>You removed the module without uninstalling</h3>

<h4>Restore the module and actually uninstall it (recommended if possible)</h4>
<h4>With an update function (recommended)</h4>
<h4>Manually with SQL (in drush sql-cli or phpmyadmin or mysqladmin)</h4>

<h3>You moved the module inside your Drupal installation</h3>

<h4>With an update function (recommended)</h4>
<h4>Manually with SQL (in drush sql-cli or phpmyadmin or mysqladmin)</h4>

Anyone feels like writing it up? If not I'll see when I have some spare time.

stefan.r’s picture

Jeroen.b yes those changes in the latest patch look good. Can we link to this documentation page in the error message as well?

jeroen.b’s picture

FileSize
716 bytes
7.5 KB
PASSED: [[SimpleTest]]: [MySQL] 41,487 pass(es). View

Created a documentation page on https://www.drupal.org/node/2487215.
Here's a new patch with a mention of the documentation page in the warning.

stefan.r’s picture

  1. +++ b/includes/update.inc
    @@ -795,6 +795,14 @@ function update_fix_d7_requirements() {
     function update_fix_d7_install_profile() {
    ...
    +  // Default profile has been renamed to Standard in D7.
    

    'Default' profile has been renamed to 'Standard' in Drupal 7.

  2. +++ b/includes/bootstrap.inc
    @@ -906,6 +923,16 @@ function drupal_get_filename($type, $name, $filename = NULL) {
    +      // Add the missing file to a temporary cache and throw an alert. This cache
    

    80 cols

  3. +++ b/modules/simpletest/tests/bootstrap.test
    @@ -410,6 +410,35 @@ class BootstrapGetFilenameTestCase extends DrupalUnitTestCase {
    +    // Get the bad records static from drupal_get_filename.
    

    s/bad/missing/
    s/drupal_get_filename/drupal_get_filename()/

  4. +++ b/modules/simpletest/tests/bootstrap.test
    @@ -410,6 +410,35 @@ class BootstrapGetFilenameTestCase extends DrupalUnitTestCase {
    +    // Searching for an item that does not exist creates a static record in drupal_get_filename.
    

    80 cols
    s/drupal_get_filename/drupal_get_filename()/

stefan.r’s picture

Issue tags: -needs backport to D7 +Needs manual testing

Status: Needs review » Needs work

The last submitted patch, 419: 1081266-419.patch, failed testing.

Status: Needs work » Needs review

stefan.r queued 419: 1081266-419.patch for re-testing.

stefan.r’s picture

jeroen.b’s picture

FileSize
2.15 KB
7.52 KB
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View

Updated the comments.

stefan.r’s picture

Issue tags: -Needs manual testing

Manually tested this.

1. The warning now says:

"User warning: The following module is missing from the file system: crumbs. For more information, see the documentation page. in drupal_get_filename() (line 934 of /Applications/MAMP/htdocs/drupal7/includes/bootstrap.inc)."

That last part isn't very informative (..in drupal_get_filename()...), and the transition from "page" to "in" seems odd, ideally we'd be pointing to wherever drupal_get_path()/drupal_get_filename() was called from, though not sure we can without debug_backtrace() magic. It will also show once for every drupal_get_filename() call, maybe we should only show each type/file combination once per request?

2. We may want to remove a file from the "missing" cache if it actually reappears. Ie. we could do the cache_get if the static is not set, unset the item in the static if it reappears and do a cache_set if ($missing !== $cache->data)

3. I'm a bit worried about the 1 day cache lifetime. I think it should be unlimited as otherwise things may be broken for a day and then suddenly work again, which could lead to a lot of head-scratching :)

Maybe the worry was that if we don't expire the cache, we mark the wrong items as missing, but that is not actually a problem, we still check in the old location with a file_exists() call before we check the missing file cache. So temporarily removing a file and then putting it back would actually be fine. It's just he moving that's problematic. That would only work after 1 day if we keep the expiry. Only fixing this upon visiting the modules/themes overview pages or upon a full cache clear (as we do now) might be the better way to go?

jeroen.b’s picture

1. Yeah, but even with some backtrace magic you won't be able to pull it off. Let's say that module_load_include called it, then the message still won't help you any further because it's just another function that's called a million times. If people want to find the calling function, I think it's best if they add the debug_backtrace manually. I can't really justify any line of debug_backtrace in code.

I think it's better to always show the PHP warning. I think developers will ignore it when we don't ("it's just one time after the cache clear... whatever"), especially if we change the cache time to unlimited.

Any idea how we can make the PHP warning better?

2. Agreed. But that might be unwanted because we have to do an unnecessary cache_get.

3. Agreed.

David_Rothstein’s picture

Status: Needs review » Needs work
  1. @@ -837,6 +837,14 @@ function drupal_get_filename($type, $name, $filename = NULL) {
       // drupal_static().
       static $files = array(), $dirs = array();
     
    +  // We use drupal_static() for the missing records so we can test it.
    +  // drupal_static_fast() is used as this function may be called often.
    +  static $drupal_static_fast;
    +  if (!isset($drupal_static_fast)) {
    +    $drupal_static_fast['missing'] = &drupal_static('drupal_get_filename:missing');
    +  }
    +  $missing = &$drupal_static_fast['missing'];
    

    Any reason not to move this code down to where it's actually going to be used (i.e., the filesystem-scanning part of the function)? Then we don't call it at all in most cases, and a regular drupal_static rather than drupal_static_fast would be fine.

  2. +        $cache = cache_get('drupal_get_filename:missing');
    

    I suspect we need some protections around this, since this could run during the early bootstrap (and looking at how other calls from inside this function are handled, they have special protections). For example, consider what happens if cache_get() is undefined, or if it throws an exception because the database isn't available yet.

  3. +    trigger_error(t('The following @type is missing from the file system: @name. For more information, see <a href="@documentation">the documentation page</a>.', array('@type' => $type, '@name' => $name, '@documentation' => 'https://www.drupal.org/node/2487215')), E_USER_WARNING);
    

    This probably shouldn't be translated here, because we want the admin to see it in their language (not necessarily the visitor's language) when viewing the watchdog logs - i.e. just like how the watchdog() function works.

    I guess we have no way to do that with trigger_error(), so maybe we could just not translate it at all for now and leave that for a followup issue.

  4. The documentation page itself looks like a good start and a good idea, although "You removed the module without uninstalling"... perhaps it should say something more like "You removed the module without disabling and uninstalling it"? (Although it's generally recommended to uninstall the module in addition to disabling it, I do not think this bug can be triggered unless you remove a module from the filesystem while it's still enabled, right?)
  5. +              set_error_handler(function($error_level, $message, $filename, $line, $context) {
    ....
    +    set_error_handler(function($error_level, $message, $filename, $line, $context) use (&$get_filename_test_triggered_error) {
    

    These won't work in PHP 5.2.

  6. +    // Generate a non-existing module name.
    +    $non_existing_module = uniqid("", TRUE);
    

    Is that even guaranteed to return a legal module name? Maybe it doesn't matter for this test, but I think $this->randomName() is a more standard way to do that.

  7.  function system_themes_page() {
    +  // Clean up the bootstrap "missing files" cache when listing themes.
    +  drupal_static_reset('drupal_get_filename:missing');
    +  cache_clear_all('drupal_get_filename:missing', 'cache');
    +
       // Get current list of themes.
       $themes = system_rebuild_theme_data();
       uasort($themes, 'system_sort_modules_by_info_name');
    @@ -788,6 +792,10 @@ function _system_is_incompatible(&$incompatible, $files, $file) {
      * @see system_modules_submit()
      */
     function system_modules($form, $form_state = array()) {
    +  // Clean up the bootstrap "missing modules" cache when listing modules.
    +  drupal_static_reset('drupal_get_filename:missing');
    +  cache_clear_all('drupal_get_filename:missing', 'cache');
    +
       // Get current list of modules.
       $files = system_rebuild_module_data();
    

    Is there a reason not to do this inside system_rebuild_theme_data() and system_rebuild_module_data() instead?

  8. I think I agree with #426.3 (that this should be a permanent cache). Introducing a cache here at all is a behavior change that we'll have to document either way (for anyone who might be using drupal_get_filename() in some kind of very dynamic situation) but having it magically expire after 1 day doesn't help that situation anyway.
  9. Speaking of the above, has anyone tested this patch with those kinds of unusual situations (I'm thinking of things like drush pm-update, or other Drush commands that change things in the file system while interacting with Drupal)?
stefan.r’s picture

1. Moving both the static and the database cache initialization into the if block that has the file scan in it (and making it a regular static) makes sense.

2. @jeroen.b see the protections around db_query() for an example :)

3. may have to remain untranslated

As to 7, the worry there is Drupal runs system_rebuild_theme_data and system_rebuild_module_data more often than necessary, so we wouldn't have as much of a performance improvement. On the other hand, it does seem safer...

stefan.r’s picture

#427:

1. Yes, I guess we may have to do without that then. As to when to show the warning, I meant once per module per request, not once after a cache clear. So it would still appear on every page, except we wouldn't see 10 warnings for the same module on top of every page.

As to the warning, I guess it's not a problem in the message itself. More so in the error handler, which shouldn't show the "in bootstrap.inc line xyz" part, and should translate the message. Maybe we can make a missing_file_error_handler()?

2. We can live with the cache_get() on every request if it only occurs if we have modules that would require a file scan as mentioned by @David_Rothstein

jeroen.b’s picture

I don't really have any time left this week to work on this, so if anyone feels like picking it up :)
@David_Rothstein, feel free to make some changes to the documentation.

stefan.r’s picture

Maybe in drush this should be merely a status message? Would it affect execution of a command if it's a user warning? (as it is right now)

Maybe we can have @moshe take a look as to whether this would cause unnecessary warnings?

stefan.r’s picture

Should this patch also be updated to account for when we're moving modules around (ie. from sites/all to sites/all/contrib? I think this is automatically fixed upon module rebuild anyway, but previously I think we were allowed to move modules to a different location than what was listed in the system table, at which point the file scan would find the new location. In such a case it's arguable whether the module is actually "missing"

Fabianx’s picture

Yes, moving modules is a problem, because sometimes you cannot even reliably clear caches (however that problem is pre-existing for other things). Can we add a test for that?

stefan.r’s picture

@Fabianx sounds good, however not sure how to deal with moved modules. If we cache the new locations of any moved files that's adding an extra layer of complexity.

Maybe we should just disallow that, and make people do a cache clear after moving a file instead?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
8.12 KB
PASSED: [[SimpleTest]]: [MySQL] 41,477 pass(es). View
7.34 KB

Current patch is untested and will still break with moved files, but should at least be a step in the right direction as it addresses some of the concerns above.

stefan.r’s picture

FileSize
8.24 KB
PASSED: [[SimpleTest]]: [MySQL] 41,491 pass(es). View
9.13 KB
EvanSchisler’s picture

When I tried to use this patch(from comment #437), it patched with no issues. However, after clearing my caches I received these notices and warnings(none of these were raised prior to the patch):

User warning: The following module is missing from the file system: imagcache_actions. For more information, see the documentation page. in drupal_get_filename() (line 942 of /home/acro/accounts/frame2/frame2/wwwroot/includes/bootstrap.inc).
User warning: The following module is missing from the file system: imagcache_actions. For more information, see the documentation page. in drupal_get_filename() (line 942 of /home/acro/accounts/frame2/frame2/wwwroot/includes/bootstrap.inc).
Notice: Undefined index: commerce_discount_product_category_has_specified_terms in commerce_discount_product_category_rules_condition_info() (line 28 of /home/acro/accounts/frame2/frame2/wwwroot/sites/all/modules/commerce_discount_product_category/commerce_discount_product_category.rules.inc).
Notice: Undefined index: commerce_order_compare_order_amount in inline_conditions_rules_condition_info() (line 37 of /home/acro/accounts/frame2/frame2/wwwroot/sites/all/modules/inline_conditions/inline_conditions.rules.inc).
Notice: Undefined index: commerce_order_has_owner in inline_conditions_rules_condition_info() (line 59 of /home/acro/accounts/frame2/frame2/wwwroot/sites/all/modules/inline_conditions/inline_conditions.rules.inc).
Notice: Undefined index: commerce_order_contains_products in inline_conditions_rules_condition_info() (line 81 of /home/acro/accounts/frame2/frame2/wwwroot/sites/all/modules/inline_conditions/inline_conditions.rules.inc).
Notice: Undefined index: commerce_order_has_specific_quantity_products in inline_conditions_rules_condition_info() (line 115 of /home/acro/accounts/frame2/frame2/wwwroot/sites/all/modules/inline_conditions/inline_conditions.rules.inc).
Notice: Undefined index: commerce_product_contains_products in inline_conditions_rules_condition_info() (line 139 of /home/acro/accounts/frame2/frame2/wwwroot/sites/all/modules/inline_conditions/inline_conditions.rules.inc).
Notice: Undefined index: commerce_product_has_specified_terms in inline_conditions_rules_condition_info() (line 163 of /home/acro/accounts/frame2/frame2/wwwroot/sites/all/modules/inline_conditions/inline_conditions.rules.inc).

jeroen.b’s picture

@EvanSchisler
That is a bug in the Imagecache Actions module, they already fixed it.
Please update the Imagecache Actions module.

See: #2166715: Typo causes extra query every page load

This is actually a good sign, because this patch finds such bugs.

stefan.r’s picture

Any further ideas about what to do when a module is moved?

I'm thinking if the data diverges from whatever we had in cache, we should just mark the module as missing and refuse to load it until caches are cleared. It's a bit of a BC break but not a very severe one, I guess the file scan was intended mostly just in case we didn't have any cached data yet -- but if we do have (faulty) data, we should just wait for that to be rebuilt instead.

jeroen.b’s picture

Yeah, agreed. We can also extend the warning message if the record was in the cache.
Maybe something like: "If you just moved this module, try to clear the cache"?

Fabianx’s picture

There is three cases of which we cover two now:

1) Something refers to a file that can never be found (typo, etc.). => trigger_error and cache in missing to avoid further file system scans.
2) Something refers to a module that was removed from the file system since => trigger_error and cache in missing to avoid further file system scans.
3) Something refers to a module that was moved => We do a file system scan every time.

Now case 3) is interesting as while it seems to "self-heal" it does so at the cost of run-time performance.

E.g. if you move a module, it will self-heal for those cases of rename using drupal_get_filename() (but e.g. registry files won't be found), but it won't be fully healed until the module system table is rebuild.

There is 2 possibilities for us here:

a) Ignore and hope caches are cleared once in a while fully => what we have now
b) Use missing not only for negative entries, but also as a 2nd level cache to cache what we found as missing

In that case $missing[$name] => FALSE is "not found" and $missing[$name] => $filename would be "found elsewhere".

In that case a file system scan would only be done when even our missing cache would be outdated.

That likely would be the perfect solution, but not sure if its in scope or not.

( There is a reason however why we don't just write-through the database in such cases as the cache is possibly database agnostic and e.g. in a memcache or redis server. )

stefan.r’s picture

I think something like b) would be the better way to go here, I'm afraid there would be even more edge cases with odd behavior with what we have now.

stefan.r’s picture

FileSize
11.59 KB
FAILED: [[SimpleTest]]: [MySQL] 41,486 pass(es), 1 fail(s), and 0 exception(s). View
4.04 KB

Maybe something like this could work? (please do not use in production yet as I haven't tested this yet, patch is just to illustrate)

stefan.r’s picture

FileSize
2 KB
12.09 KB
FAILED: [[SimpleTest]]: [MySQL] 41,493 pass(es), 1 fail(s), and 0 exception(s). View

I had forgotten to implement reading of the cached file scan result in that previous patch. This patch still should not be used in production until this is further tested/reviewed...

The last submitted patch, 444: 1081266-444.patch, failed testing.

stefan.r’s picture

Looking at the whole patch, it needs a bit of further work to deal with the case where the cache_get('drupal_get_filename:missing') fails/is unavailable in early bootstrap. Just resetting the missing static during the database bootstrap doesn't feel like the right solution.

There's also a couple of trailing spaces.

Status: Needs review » Needs work

The last submitted patch, 445: 1081266-445.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
12.27 KB
PASSED: [[SimpleTest]]: [MySQL] 41,483 pass(es). View
3.06 KB
5.7 KB
Fabianx’s picture

Looks really great!

Fabianx’s picture

Issue summary: View changes

Added myself to proposed commit message

stefan.r’s picture

FileSize
15.44 KB
PASSED: [[SimpleTest]]: [MySQL] 41,492 pass(es). View
9.34 KB

Well-deserved, thanks again for the helpful reviews :)

Adding some documentation

stefan.r’s picture

FileSize
15.79 KB
PASSED: [[SimpleTest]]: [MySQL] 41,501 pass(es). View
2.23 KB

And this removes the cached "missing" record if a missing file reappeaars.

Fabianx’s picture

+++ b/includes/bootstrap.inc
@@ -986,7 +989,13 @@
+    if (isset($missing[$type][$name]) && $missing[$type][$name] === TRUE) {

Arguably its an edge case: If the module gets moved back, we don't remove it from missing cache?

stefan.r’s picture

stefan.r’s picture

FileSize
786 bytes
15.89 KB
PASSED: [[SimpleTest]]: [MySQL] 41,487 pass(es). View

Maybe this would work?

stefan.r’s picture

+++ b/includes/bootstrap.inc
@@ -897,15 +963,80 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+          trigger_error(format_string('The following @type has moved on the file system: @name. Clearing caches may help fix this. For more information, see <a href="@documentation">the documentation page</a>.', array('@type' => $type, '@name' => $name, '@documentation' => 'https://www.drupal.org/node/2487215')), E_USER_WARNING);

Can we think of better verbiage here? It should probably explain the data in the system table is outdated and that that is what a cache clear might fix.

stefan.r’s picture

Just wondering about #428.8, ie. the behavior changes posed by the current patch...

a) If we remove a file altogether, we will cache that it's missing and cease to do any further file scans until a module/theme rebuild happens (such as during cache clears and when visiting admin pages). This means that once a file has been marked as missing, we can't put it back in another location anymore but we can put it back in the location logged in the system table, at which point everything will be OK again and the file will be removed from the missing cache.

b) If we move a file from wherever it was supposed to be, we will cache the result of the file scan and use that instead of a file scan from then on. If the file then moves again from its new location, we will do a new file scan, and cache the result of that. However, if we move a file back to where it was supposed to be, everything is OK again and we remove the record from the missing cache.

In the case of moving a file, the "missing" caching is only relevant as long as we haven't done a rebuild. The rebuild should log the new location into the system table.

So outside of whatever is happening in drupal_get_filename(), the only limitation/behavior change that this patch introduces is that now we can't first delete a file, and then put it back in another location (as it has been marked as missing already, so we don't do a file scan). Reactivating the file would require either putting it back in its original location first, or performing a cache clear.

Anything I'm missing here?

David_Rothstein’s picture

This is getting complicated. If a file moves, and we know what the old location and the new location are, why do we need to put that in a cache? Shouldn't we really just do something like this:

  db_update('system')
    ->fields(array('filename' => $new_filename)
    ->condition('filename', $old_filename)
    ->execute();
  system_list_reset();

Not sure if we'd want to do that inside drupal_get_filename(), but even if not we could do it at the end of the page request (like the patch is currently doing for the cache).

Maybe in drush this should be merely a status message? Would it affect execution of a command if it's a user warning? (as it is right now)

Interesting question. That plus some other reasons makes me wonder if we should go back to using watchdog() after all (rather than trigger_error()). When I originally suggested trigger_error() it was based on a couple things:

  1. That we'd only be adding this warning (no cache), i.e. that it would indicate a major ongoing performance problem with your site. But the consensus here is now to add the cache too, so this warning no longer indicates such a major problem.
  2. I was thinking mostly about #2380361: Latest tweaks to bootstrap.inc can make Drupal sluggish when there is code that tries to load a file that doesn't exist (problems in code that we want to warn a developer about in realtime). While that's still true, the fact that it's also common to get into this situation because of something the site owner did means that a regular watchdog() call might be OK.

@David_Rothstein, feel free to make some changes to the documentation.

OK, I did https://www.drupal.org/node/2487215/revisions/view/8451805/8483001 to address the disabled/uninstalled issue.

stefan.r’s picture

I agree we want to simplify the current patch as much as possible, so updating the system table at the end of the request with the moved modules that had been statically cached could potentially work, although it could cause a bit of a slowdown on subsequent requests and be more of a BC break than just doing the caching: the data in the system can now change on any regular request as opposed to just after a rebuild. The other worry is we may not catch all the moved files so this may turn out to be spread out over several requests. @Fabianx what do you think?

As to just doing a watchdog() call, the problem is it's not annoying enough for people to clean up their stuff and people can easily miss/ignore it. A moved module may not be a big deal, but a missing one is. And even with the cache, the first few requests after a missing module cache clear are still going to be very slow as we re-do the file scans.

Ideally we want to nag administrators and developers as much as possible. Maybe instead of a trigger_error() we should do a drupal_set_message() (in addition to a watchdog() call)?

Fabianx’s picture

I think the cache is actually pretty nice for such reasons.

Yes we might want to change the code to be simpler, but writing the system table with partially new information is likely not the right choice. And doing dynamic rebuilds - like menu_get_item() vs. menu_rebuild() vs. menu_rebuild_needed() has a ton of own problems ...

I still think a cache layer is a good thing to take care of what we would have done anyway (falling back).

It just means we cache the result of the fallback file system scan - which should be fine.

Maybe moving the file_system_scan to its own function would make this all clearer, and allow clearer wrapping of it?

--

drupal_set_message() for something like this is a no-go -- end users see it and that is problematic ...

stefan.r’s picture

Thinking this over a bit further I think you may be right here as far as the moved file caching is concerned.

I agree the drupal_get_filename() function is getting way to long, taking out the file system scan part would be a good start at making it clearer.

As to the status message, I was thinking we wrap the drupal_set_message() in an if statement so only admins and developers see it, but we don't really have a way of determining that do we? So it'd really have to be trigger_errror() if we want to annoy a bit more than just a watchdog message?

stefan.r’s picture

Just to clarify, other than potentially disrupting Drush, are there any further concerns with a trigger_error()? Maybe we could live with a Drupal message to users with permission 'administer site configuration' only?

David_Rothstein’s picture

I don't fully understand what's wrong with updating the system table when we know it's out of date. Essentially, that already is our cache of where known files are in the filesystem (and it already has other caches on top of it), right? Which is what makes me a little uneasy about adding a whole separate cache for the case of a module/theme that has moved somewhere else.

But perhaps moving this to a separate function as suggested above would make it cleaner and less complex.

As to the status message, I was thinking we wrap the drupal_set_message() in an if statement so only admins and developers see it, but we don't really have a way of determining that do we?

Could probably do something to check the 'error_level' variable, like error_displayable() I suppose - but yeah, it's not ideal.

Just to clarify, other than potentially disrupting Drush, are there any further concerns with a trigger_error()?

Not really - a minor concern about the lack of translatability (mentioned earlier) but it's not too bad.

And a general concern that we're going to see lots of support requests from people who suddenly see this message displaying on the screen on their production sites. They shouldn't have set their production site up to display errors to the screen in the first place, but the reality is, I bet there's a lot of overlap between sites that don't direct error messages to the logs on production and sites that delete modules from the filesystem without disabling them :)

stefan.r’s picture

Hmm the last concern seems like the more serious one. A substantial amount of installs probably doesn't have that configured right. And I guess the same concern would still apply if we say that error_level must be ERROR_REPORTING_DISPLAY_ALL (even if we recommend people not to):

  $form['error_level'] = array(
    '#type' => 'radios',
    '#title' => t('Error messages to display'),
    '#default_value' => variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL),
    '#options' => array(
      ERROR_REPORTING_HIDE => t('None'),
      ERROR_REPORTING_DISPLAY_SOME => t('Errors and warnings'),
      ERROR_REPORTING_DISPLAY_ALL => t('All messages'),
    ),
    '#description' => t('It is recommended that sites running on production environments do not display any errors.'),
  );

On the other hand, the error message is a disturbance but a minor one; as soon as the missing module is dealt with, the error disappears. And in the link in the error message we can also have a "Why do end users see this error message?" heading where we point people to wherever they can set the error_level to "Errors and warnings" or "None".

I'll let others have some input on this as I can see the arguments both for only displaying this to logged in administrators (to prevent breaking a number of misconfigured sites), but also for making it a user level notice/warning and potentially breaking a few sites, but pushing a much larger number of sites to fix their stuff

stefan.r’s picture

As to fixing the system table in real time, I'll let Fabianx chime in but my main concern was that it felt like a substantially more invasive change than just caching the file scan result.

And yes, in a way it's a cache, but it's a pretty important one compared to other caches, and updating it in real time feels like making it easier than it should be to change whatever is in there (which should be leading, and not what may be -temporarily- happening on the file system), or to only do a partial update when several files have moved. Having to wait for a rebuild means less things can go wrong in the mean time, ie. when one person passes a database dump to someone else with almost the same files installed, except this one module is duplicated in two locations on one system but only in one location on the other.

stefan.r’s picture

Which makes me think of another edge case that would be a change from current behavior, both in the case of caching the result of the file scan and in the case of changing the system record directly: a module being installed in more than one place, ie. in sites/all as well as in the profile folder.

For instance:

  1. We move a file from its original location (listed in the system table) to somewhere else, let's say from sites/all/modules to profiles/some_distribution/contrib/modules
  2. The file scan/updated system table will cache the new location
  3. We install a slightly different version of this file in another location that would have had prevalence in the file scan (profiles/some_distribution/modules?), and now have two versions of this module on our file system.

...and I could see something like this happening when people symlink a distribution for different sites but want to change a module for one site only.

Result without the patch: Drupal would have used the module in the third location as it would do a file scan every time.

Result with the patch: Drupal will use the module in the second location (as it caches the result of the file scan), and only after a module rebulid, would it go back to using the file in the third location.

Fabianx’s picture

My main concern is that the system_rebuild_module_data() would rebuild the data for the full module at once (and also take care of registry, etc. potentially if done via drush cc all), while this partial update of system table would only do for the accessed files. The system table is then out of sync.

It would be a different story if we rebuild the system table for this whole module / directory. (if that is possible)

==

Error messages:

Yes, trigger_error is potentially problematic, but given a good message and a warning in the release notes, that should be okay.

==

One module at two locations at once:

- The behavior has always been undefined, therefore we can change the behavior without problems.

stefan.r’s picture

And scheduling a complete rebuild for the next request, such as variable_set('system_rebuild_needed', TRUE) (as opposed to just system_list_reset()) is also going to be problematic I guess, the rebuild can take quite a while...

David_Rothstein’s picture

There is only one row per module in the system table, so I still don't fully understand most of that concern. For each module, either we know the correct location or we don't. (And if the concern is about related modules, e.g. multiple submodules in the same package, note that in practice all enabled modules will be touched in the same page request anyway, at least for uncached page requests.)

Having to wait for a rebuild means less things can go wrong in the mean time, ie. when one person passes a database dump to someone else with almost the same files installed, except this one module is duplicated in two locations on one system but only in one location on the other.

OK, yeah, I can buy that we'd like to avoid any problems with that.

stefan.r’s picture

...which reminds me, we should be checking if a module/theme has moved in system_list() (where the drupal_get_filename() cache gets primed after a module_load_all() under normal circumstances).

We may also need to look what to do for theme_engine files, I don't think those locations are stored in the system table?

I'm willing to investigate if updating the system table directly is feasible if @Fabianx thinks it's fine after @David's reminder that all enabled modules are loaded on every request. But there's not a single edge case where (moved) disabled modules/profiles could be a problem? (I think we already loads disabled themes on every request?) And as far as core is concerned I believe the system table should only hold module and theme files, can it hold anything else?

stefan.r’s picture

Also, moving a file may still leave the menu_router include_file data and the registry data inconsistent, but that would have happened before the patch as well, right?

Fabianx’s picture

Yes, if there was a chance to fix that, that would be great though :-D, but the problem is that then we need a lot of locking to ensure we don't get new races, while with the current cache layer the worst that can happen is that we do another file scan ...

stefan.r’s picture

Discussed the system table updating at the end of a request on IRC with @catch and @Fabianx and I think I'm going to explore taking out the file scan and caching its result a bit further first, maybe we can make that simpler after all...

stefan.r’s picture

FileSize
21.68 KB
19.9 KB
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
Fabianx’s picture

Overall looks much better now, will have a closer look again tomorrow - hopefully.

stefan.r’s picture

Thanks for having a look. Just a few questions to look into when you do:

  1. +++ b/includes/bootstrap.inc
    @@ -850,192 +850,222 @@
    +function _drupal_get_filename_fallback($type, $name) {
    

    Maybe we can we think of a better name for this.

  2. +++ b/includes/bootstrap.inc
    @@ -850,192 +850,222 @@
    +      $files[$type][$matched_name] = $file->uri;
    

    It's not too confusing that this is called the same as the leading static in drupal_get_filename()?

  3. +++ b/includes/module.inc
    @@ -121,6 +121,7 @@ function module_list($refresh = FALSE, $bootstrap_refresh = FALSE, $sort = FALSE
    +  $system_filepaths = &drupal_static('system_filepaths');
    

    Are we fine with the way this is named?

stefan.r’s picture

FileSize
19.09 KB
PASSED: [[SimpleTest]]: [MySQL] 41,482 pass(es). View
4.38 KB

Some nitpicks, and moved the cache clearing to system_list_reset()

stefan.r’s picture

FileSize
9.83 KB
19.78 KB
PASSED: [[SimpleTest]]: [MySQL] 41,480 pass(es). View

Updated some comments

stefan.r’s picture

FileSize
19.73 KB
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
2.03 KB
joseph.olstad’s picture

Tested patch 480 481.

Applied patch.
installed test contrib module
removed test contrib module from the filesystem

clear cache, no notices

drush updatedb
WD system: The following module is missing from the file system: test. [error]
No database updates required [success]

then did some basic performance profile of a cache clear.

Missing module from filesystem test took 43 seconds to perform the cache clear.

Restored test module to the filesystem.

Performed another cache clear.
Test configuration took exactly 43 seconds to perform the cache clear, same as when files were missing.

From these rudimentary tests I can confirm that patch 480 481 is working as expected.

Thanks

RTBC+

stefan.r’s picture

Thanks for the manual testing. So Drush ought to be fine then, I guess it's only during a "drush make" where an error may be problematic? (and a user-level PHP warning may still be fine)

I also noticed this triggers a missing file warning on every request, but only triggers a moved file error once, so we should probably update the patch to warn on every request with moved files as well.

I think the ability to move modules may actually be an unintended consequence of the file scan, because even if the drupal_get_filename()/drupal_get_path() call works correctly for a moved module, the registry / menu router records still remain broken.

stefan.r’s picture

FileSize
19.92 KB
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View
3.32 KB

This should show the "moved file" warning every request.

joseph.olstad’s picture

Hi stefan.r 481 was sufficient for my tastebuds (warning when running updatedb, no problem with drush make, I believe that drush make confusion may have been refering to some earlier discussion about how some distributions of drupal might forget to pm-uninstall between versions while removing a feature or custom or contrib module), hence the need for this issue to get a final patch to commit and into core.

joseph.olstad’s picture

(same message as above)

David_Rothstein’s picture

Only skimmed it, but the new approach looks cleaner so far!

We probably need to test this with the Update Manager also. In Drupal 8, this is causing an issue there because the Update Manager code was relying on drupal_get_path() to check whether a module/theme exists in the filesystem or not (see #2042447-133: Install a module user interface does not install modules (or themes)). I think Drupal 7 might have a similar issue...

joseph.olstad’s picture

Ok, I re-ran a similar testplan outlined in comment #481 however this time instead of removing only one test module I removed 4 features/modules from the filesystem thinking that I might notice bigger performance differences (however I only sampled a half dozen times or so.

Results: between 38 and 47 seconds for a cache clear. The higher number came when the patch was rolled back(reversed). I ran more cache-clears afterwards and could not get the higher number again.

Visual tests were done (no perceptible performance differences between tests with and without patch) - xhprof would come in handy about now.

The patch otherwise working as expected, the only thing I could say is that the drush message includes HTML markup that isn't pretty in the drush command line mode.

Here's what the output looks like:

PHP warning:   The following <em class="placeholder">module</em> is missing from the file system: <em class="placeholder">rules</em>. In order to fix this, put the file back in its        [warning]
original location or uninstall the module. For more information, see <a href="https://www.drupal.org/node/2487215">the documentation page</a>. bootstrap.inc:1000
The following <em class="placeholder">module</em> is missing from the file system: <em class="placeholder">rules</em>. In order to fix this, put the file back in its        [warning]
original location or uninstall the module. For more information, see <a href="https://www.drupal.org/node/2487215">the documentation page</a>. bootstrap.inc:1000
The following module is missing from the file system: rules. In order to fix this, put the file back in its original location or uninstall the module. For more information, [error]
see the documentation page. in _drupal_get_filename_fallback() (line 1000 of /var/www/html/d/includes/bootstrap.inc).
The following module is missing from the file system: rules. In order to fix this, put the file back in its original location or uninstall the module. For more information, [error]
see the documentation page. in _drupal_get_filename_fallback() (line 1000 of /var/www/html/d/includes/bootstrap.inc).

Also, for some reason it looks like the same/similar notice shows up twice.

It'd be nice to pull out xhprof and run benchmarks with and without the patch 483, and compare that to patch 102 (which we're currently using in production thanks). This would give us a better idea of where we're at in terms of performance and have some sort of benchmark set up.

My personal server is one of the better places to test this because it's load doesn't fluctuate very much and it's on a physical box with predictable performance/availability.

For now, aside from the markup on the text, I don't have any concerns other than wanting a full xhprof performance diagnosis.

The text output in 483 differs from 481 in the html tagging of the drush text output added to 483.

We've come so far, almost to the finish-line, please help!

stefan.r’s picture

@David_Rothstein regarding the updater, D7 does also have a ModuleUpdater::getInstallDirectory() and ModuleUpdater::isInstalled() that do a drupal_get_path(), but I'm not sure those should be triggering a (partial) cache clear. As opposed to D8 (where we dispose of the file scan), in the current D7 patch we merely cache the result of the file scan, which as far as the update manager is concerned we should probably stick with, for consistency. So I gues the only thing we want to test is that it doesn't trigger any "missing file" errors where it shouldn't?

@joseph.olstad I am not sure that comparing with #102 will help us further along here -- both patches skip the file scan so the performance should be the same.

The user warning will show up once for every call to drupal_get_filename(). Indeed as mentioned earlier we should probably make that be once every request instead.

As to the drush message, what do you suggest we do about it? Maybe a separate issue (in Drush or in Drupal core) where we make the error handler show cleaner messages on CLI?

Fabianx’s picture

#488: The HTML message thingy is out-of-scope here, but just doing it once per request should be fine.

stefan.r’s picture

Yes, that was what I was suggesting as well, i.e. keep the HTML in the message, and solve the CLI HTML filtering in another issue. It could even be fixed in Drush itself.

stefan.r’s picture

FileSize
3.08 KB
20.94 KB
PASSED: [[SimpleTest]]: [MySQL] 41,491 pass(es). View

Update manager seems fine.

I don't think there's actually a case where it would throw an unnecessary error as it doesn't allow for installing previously non-existing modules anyway. I don't think it needs to be (partially) clearing any caches either -- if the rest of drupal considers the module to be in a certain place, the update manager can assume that too. And if it's missing, it won't be trying to update it anyway. wrong, I forgot about the installer

Added a "one error per request" function.

stefan.r’s picture

FileSize
1.03 KB
20.94 KB
PASSED: [[SimpleTest]]: [MySQL] 41,488 pass(es). View

Removing a trailing space

Fabianx’s picture

Status: Needs review » Needs work
+++ b/includes/bootstrap.inc
@@ -992,8 +992,35 @@
+  static $errors_created = array();
+  if (empty($errors_shown[$type][$name][$error_type])) {
...
+    $errors_created[$type][$name][$error_type] = TRUE;

$errors_shown vs. $errors_created won't fly ...

David_Rothstein’s picture

@David_Rothstein regarding the updater.... So I guess the only thing we want to test is that it doesn't trigger any "missing file" errors where it shouldn't?

Right, that's what I meant. Thanks for checking. I'm surprised it doesn't trigger an error actually - the code in the Update Manager that does this looks very very similar in Drupal 7 and Drupal 8 so I would expect that in both cases it calls drupal_get_path() on a nonexistent module and therefore triggers the error message.

stefan.r’s picture

@David_Rothstein well maybe I was doing this wrong then, perhaps a silly question but at what point would that be calling a drupal_get_path() on a nonexistent module?

The thing I tested was updating an existing module - should that call a drupal_get_path() on a nonexistent module at any point?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
887 bytes
20.94 KB
PASSED: [[SimpleTest]]: [MySQL] 41,483 pass(es). View
stefan.r’s picture

Ah, I had overlooked admin/modules/install -- let me test that.

stefan.r’s picture

FileSize
5.9 KB
22.65 KB
PASSED: [[SimpleTest]]: [MySQL] 41,486 pass(es). View

Indeed that was triggering an unnecessary error, here's a try at fixing that.

stefan.r’s picture

FileSize
22.64 KB
PASSED: [[SimpleTest]]: [MySQL] 41,484 pass(es). View
1.43 KB

And we can actually keep casting to boolean as (bool) NULL === FALSE

stefan.r’s picture

Anyone willing to review/test this? :)

dagmar’s picture

I ran a quick search and it seems the pattern cache_get('_cache_identifier') is not used anywhere in core.

Maybe you could use a more descriptive value for that cache ID. For reference here is the list of the current values for the cache_bootstrap table:

  • bootstrap_modules
  • system_list
  • module_implements
  • hook_info
  • variables
  • lookup_cache
stefan.r’s picture

A lot of those use the function name though. This one just happens to begin with an underscore.

How about we rename it to file_scans?

mikeytown2’s picture

Just gave #499 a spin and it seems the cache it not written under all circumstances; even if drupal was fully bootstraped.

Put this in a php file like test.php at the root level; note that REQUEST_TIME_FLOAT requires php 5.4

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
$runtime = round(microtime(TRUE) - $_SERVER['REQUEST_TIME_FLOAT'], 5);
echo "Drupal bootstrap took $runtime seconds\n<br>";

Run it a couple of times to get baseline on how long drupal takes. Remove a module and run test.php again a couple of times. Visit a normal drupal page like /admin, then hit test.php and should get faster.

Why doesn't test.php get faster after the 2nd run, why do I need to run menu_execute_active_handler() for this to work; drupal_deliver_html_page()? What about Drupal acting as a backend for something else?

Making sure we're OK with this or if we want to look over what we have currently; maybe use a shutdown function? Then again looking at module_implements_write_cache() makes me think this is in the correct place...

stefan.r’s picture

because the cache is written to in drupal_page_footer()

Fabianx’s picture

#503: Yes, the benchmark needs to include a call to drupal_page_footer(), that is normal and needs to be used for bootstrap testing, also for module_implements testing ...

mikeytown2’s picture

RTBC from me

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Lets do it!

pwaterz’s picture

First off, thanks so much for all the hard work on this! I have a question and I can't quite make out based on the book of comments here. Will this patch prevent modules from being disabled if the module goes missing? Like if I put the module back in place, will it re enable it self?

marcelovani’s picture

Nothing to be worried about pwaterz, it will check again anytime the cache is cleared

fgm’s picture

Status: Reviewed & tested by the community » Needs work

Funny, I just happened to notice this issue yesterday diagnosing a slow D7 customer site : I had not seen a site with removed/moved modules for years.

  1. +++ b/includes/bootstrap.inc
    @@ -847,68 +854,269 @@ function drupal_get_filename($type, $name, $filename = NULL) {
    -  // Verify that we have an active database connection, before querying
    

    I think the patch may be ignoring one case: it assumes the DB can only be unavailable during an install of it the DB can not be reached. However, I suspect that with $conf['page_cache_without_database'] = TRUE, combined with $conf['page_cache_invoke_hooks'] = TRUE (it is usually set to FALSE in that case), this might be triggered to look for hook implementations after a non-DB cache flush. This probably needs to be tested to be sure the case is covered, but even if it works, the comment leading to the exception would then need to be completed by this case.

  2. +++ b/includes/bootstrap.inc
    @@ -847,68 +854,269 @@ function drupal_get_filename($type, $name, $filename = NULL) {
    +      catch (Exception $e) {
    

    Since this patch adds an exception, the phpdoc should be updated with a @throws.

stefan.r’s picture

+++ b/includes/bootstrap.inc
@@ -847,68 +854,269 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+ catch (Exception $e) {

I don't think #2 is needed? drupal_get_filename doesn't throw an exception does it?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
22.8 KB
PASSED: [[SimpleTest]]: [MySQL] 41,497 pass(es). View
762 bytes
fgm’s picture

@stefan.r : my bad, you're right. If the patch keeps this pokemon catch, any exception will not surface, so there is nothing to add.

I cannot but wonder if it is really sane to catch all the exceptions which can be raised by db_query, though. Because, although this is not documented, db_query can throw exceptions, at least \DatabaseConnectionNotDefinedException and DatabaseDriverNotSpecifiedException from \Database::openConnection and \PDOException from the \DatabaseConnection::query() method, and catching \Exception is normally seen as bad practice.

stefan.r’s picture

Well yes... it's not pretty but in practice I can't see this doing any damage. Considering the same catch is in the current function as well and we merely leave it unchanged, getting that exactly right would be out of scope for this patch.

Let's re-RTBC if you agree with the updated comment in #512?

fgm’s picture

Status: Needs review » Reviewed & tested by the community

Indeed.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

The patch is complicated but well-written and well-documented. I'd like to get it in, but I tested it and it didn't seem to fully work:

  1. When I tried moving a module in the filesystem, the code still does a filesystem scan on every page request, and also results in a PHP notice: Undefined index: module in _drupal_get_filename_fallback() (line 985.
  2. When I tried calling drupal_get_path() on a module that exists in the filesystem but is not yet in the {system} table, the code does a filesystem scan on every page request, and never displays the message.

I think some of this might be due to the 'system_filepaths' static variable, the logic of which I found hard to follow and I'm not sure it's working correctly.

Do we even need that variable and the logic surrounding it? I know we've been through a few iterations here and there are a lot of edge cases we're trying to handle, but I'm trying to remember if there's a reason we couldn't use simpler logic. For example, leave the main part of drupal_get_filename() alone, but at the end, before performing a filesystem scan:

  1. Do a cache_get() + merge into the static variable (only once per page request).
  2. Check the static cache. If there's a record of the item in the cache, use it if it's either (a) FALSE (missing module) or (b) a location for which file_exists() returns TRUE.
  3. Otherwise, do the filesystem scan. If the module is missing, store FALSE in the static cache. If the module is found (i.e. a moved or newly-appeared module), store its location in the static cache. In both cases, set #write_cache to trigger a cache_set() at the end of the page request.
  4. Trigger errors if requested, "missing" if the result is FALSE or "moved" (actually a bit misnamed) otherwise. Note that we should probably skip triggering the "moved" error in the case where the db_query() on the {system} table was unable to run (e.g., database down/installer situation).

Would anything be missing if we simplified it like the above?

Some other things:

  1. I think we should have a change notice for this issue, to cover things like:
    -    return (bool) drupal_get_path('theme', $this->name);
    +    return (bool) drupal_get_filename('theme', $this->name, NULL, FALSE);
    

    and:

    -              if (!drupal_get_filename('module', $module)) {
    +              // Pass FALSE as fourth argument so no error gets created for
    +              // the missing file.
    +              $found_module = drupal_get_filename('module', $module, NULL, FALSE);
    

    I'm still a little wary of those changes, but the patch seems to catch every case in core that needs it, and I couldn't find any in contrib after grepping through a few popular modules. Plus the worst that happens is a trigger_error() anyway, so it should be OK.

  2. The documentation page at https://www.drupal.org/node/2487215 looks like it needs a lot of updating (lots of @todos). It could also be simplified - I'm not sure we ever need to document writing an update function, and for the "moved module" case shouldn't a cache clear always be the recommendation?

    We probably also need a section on "the module never existed in the first place" (e.g. where this error appears due to a bug in a contrib module). Most of the first paragraph could move to this section; also, the reference to module_exists() in the first paragraph doesn't look right - I don't think this issue is necessarily limited to enabled modules.

  3. + * @param boolean $create_errors
    + *   Whether to create an error when a file is missing / moved. Defaults to
    ....
    + * @param boolean $create_errors
    + *   Whether to create an error when a file is missing or moved.
    

    I think this would be better named $trigger_error (and documented as "whether to trigger an error"). Also I think it should be @param bool rather than @param boolean?

  4. +      trigger_error(format_string('The following %type has moved on the file system: %name. In order to fix this, clear caches or put the file back in its original location. For more information, see <a href="@documentation">the documentation page</a>.', array('%type' => $type, '%name' => $name, '@documentation' => 'https://www.drupal.org/node/2487215')), E_USER_WARNING);
    

    "moved within the file system" might be better wording here. Although based on the above discussion, it could be better to say "has moved or has recently appeared in the file system"?

  5. +  // @see system_update_7049()
    

    Should be a complete sentence since this is an inline code comment (not PHPDoc), e.g. "See system_update_7049()."

stefan.r’s picture

Aren't 1 and 2 the intended behavior (other than the notice)? Let me have a look.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
5.92 KB
22.91 KB
PASSED: [[SimpleTest]]: [MySQL] 41,619 pass(es). View

I don't know that we need to be catering to use case #2 (drupal_get_filename on a module not yet in the system table) - we hadn't thought of it earlier but it sounds like enough of a edge case for it to be out of scope here... we can keep the file scan there I think.

As to the notice, that seems very odd. How can I reproduce this? Merely installing a module and moving it doesn't work. Are you calling drupal_get_filename() before system_list() had been called? (in module_load_all()). That's where the static is being built...

As to the proposal of getting rid of the system static entirely, I'm not too sure. I remember discussing this with Fabianx/catch(?) and I think it's better to keep it. The problem is we want to know if something has moved relative to the path in the system table which we consider "leading" (see earlier comments). Removing that feels like too drastic of a change.

Uploading a new patch that incorporates the feedback. It seems the file scan for moved modules wasn't being cached properly indeed :)

stefan.r’s picture

stefan.r’s picture

Actually I misread the line number... 985 was not about the system static but about the $file_scans static. That should now be fixed with the latest change:

+++ b/includes/bootstrap.inc
@@ -982,14 +982,14 @@
-      if ($file_scans[$type][$name] != $filename_from_file_scan) {
+      if (!isset($file_scans[$type][$name]) || isset($file_scans[$type][$name]) && $file_scans[$type][$name] != $filename_from_file_scan) {

This tells us to write to persistent cache if it's a new "moved" record as well (i.e. when $file_scans[$type][$name] is not yet set)

I did manage to reproduce the notice with the old patch (on a bootstrap module - not sure if that mattered), but with the new patch this seems to work.

David_Rothstein’s picture

My testing was with a non-bootstrap module (I used the Diff module). Here are the steps I took:

  1. Install Drupal core with the patch applied and debugging added to _drupal_get_filename_perform_file_scan() to track when it's called.
  2. Install the module, clear caches, and visit the homepage.
  3. Move the module (e.g. from sites/all/modules/contrib to sites/all/modules) and reload the homepage a few times.

The issue was reproducible with #512 but I can confirm that #518 fixes it.

As to the proposal of getting rid of the system static entirely, I'm not too sure. I remember discussing this with Fabianx/catch(?) and I think it's better to keep it. The problem is we want to know if something has moved relative to the path in the system table which we consider "leading" (see earlier comments)

I saw the comments, but I guess it's just not clear to me how the system static relates to them. With the code already in drupal_get_filename() before this patch, we know that the only time we run a file scan is when the info from the {system} table is unavailable, missing, or out-of-date. Given that we already do that, I'm not sure what else we get from the static variable. Can anyone think of a (manual) test scenario where the static is expected to be necessary?

David_Rothstein’s picture

The change record and documentation page are looking good. I made a few more updates to them now, and I think they are probably set.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Btw. another module leading to a file scan on each drupal_load(): flickrapi (fixed in their -dev versions already), but the module_load_include with a typo really puzzled us at first ...

So good to see this moving forward fine.

stefan.r’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.85 KB
23.3 KB
FAILED: [[SimpleTest]]: [MySQL] 41,609 pass(es), 2 fail(s), and 0 exception(s). View

Implementing the suggestion in #521 to see what it looks like.

Not sure it's a simplification but I don't see any big problem with it either.

Status: Needs review » Needs work

The last submitted patch, 524: 1081266-524.patch, failed testing.

David_Rothstein’s picture

Hm, I thought it might be simpler than that actually. I will try to take a look at this later and see.

I guess another part that isn't clear to me is why all the code at the bottom of drupal_get_filename() that modifies &drupal_static('_drupal_get_filename_fallback') is necessary? It seemed to me like that logic is (or at least could be) handled by _drupal_get_filename_fallback() itself.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
23.61 KB
FAILED: [[SimpleTest]]: [MySQL] 41,606 pass(es), 0 fail(s), and 3 exception(s). View
0 bytes

Let's see if this fixes the test fail, the trigger_error() message was being suppressed because the system table didn't exist.

As to the code at the bottom of drupal_get_filename(), it's necessary just to keep the cached list up-to-date and remove entries that used to be marked as moved or missing which have reappeared at their original locations.

If the modules reappear, a call to drupal_get_filename('reappeared_module') won't go through _drupal_get_filename_fallback(). This was the case in the previous version of the patch as well FWIW.

@David_Rothstein expressed a concern about the logic in the current code being too different to the previously RTBC'ed code... could anyone give the changes a close review and see if this should actually be a worry?

Status: Needs review » Needs work

The last submitted patch, 527: 1081266-527.patch, failed testing.

stefan.r’s picture

Hmm looking at these failures I wonder if we should go back to the system table check solution that had been RTBCed 4 months ago. It's not that complicated really - it just caches the result of the 2 system table checks we do in core.

Not sure catering to the "newly introduced file" use case is worth the effort and additional risk here... as that would be fixed by a cache clear anyway and is not as common.

David_Rothstein’s picture

Sorry for the delay. I'm going to take another look at this tomorrow.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
28.54 KB
PASSED: [[SimpleTest]]: [MySQL] 41,625 pass(es). View
34.18 KB

OK, I tried reworking the patch and think I may have succeeded in simplifying it. All the logic is now self-contained underneath _drupal_get_filename_fallback() and reasonably straightforward, and all the various cases should be handled.

The patch is bigger than the last one, but only because I expanded the tests also. (I'm posting the interdiff too, but the patch itself is probably easier to review.)

As to the code at the bottom of drupal_get_filename(), it's necessary just to keep the cached list up-to-date and remove entries that used to be marked as moved or missing which have reappeared at their original locations.

If the modules reappear, a call to drupal_get_filename('reappeared_module') won't go through _drupal_get_filename_fallback().

But if _drupal_get_filename_fallback() doesn't get called, why do we care about its cache being up to date? It doesn't have to be (and can't be) an accurate record of exactly what's in the filesystem - it's just a cache of what we found the last time we scanned the filesystem.

The only case I can think of where this would make a difference would be if you deleted a module, then restored it to its original location, then moved it to a new location (all without a system_list_reset() anywhere in between). But the earlier patches don't actually deal with that either (I tested this with #527) since they don't do a cache_get() at the bottom of drupal_get_filename() (and presumably shouldn't, for performance reasons). The cache doesn't actually get updated in those cases either, right?

It also seems fine not to deal with that. I think you all came to a similar conclusion earlier - we need to support putting a module back in its original location (so the site can recover after something goes wrong) and we need to support moving a module too, but we shouldn't have to support any more complicated scenarios because it's reasonable to assume you need to visit admin/modules or clear caches after making big changes to the filesystem.

David_Rothstein’s picture

FileSize
28.49 KB
PASSED: [[SimpleTest]]: [MySQL] 41,637 pass(es). View
473 bytes

Removing a line of code that was probably a bad idea on my part...

stefan.r’s picture

This seems well thought out, it's quite a bit simpler and has extra test coverage, so +1.

I agree we can live with the cached file scans not being completely up to date - it changes behavior a bit in some edge cases, but those would be solved through a system_rebuild_module_data.

+++ b/includes/bootstrap.inc
@@ -847,64 +854,253 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+function &_drupal_file_scan_cache() {

Is the & needed here?

stefan.r’s picture

Should we also have a prominent reminder in the release notes about turning off error messages on production systems and about this patch potentially causing thousands of entries in the error log?

David_Rothstein’s picture

Thanks for the review! Anyone able to do some more review or testing of this?

+function &_drupal_file_scan_cache() {

Is the & needed here?

I believe so since it's what allows functions that call this to modify the cache. (Same reason that the definitions of drupal_static() and batch_get() have it.)

Should we also have a prominent reminder in the release notes about turning off error messages on production systems and about this patch potentially causing thousands of entries in the error log?

Yeah, good idea. Regarding the thousands of entries in the error log, I wonder if there's a way to make it fewer? But the only obvious way I can think of would be to show the message once after the cache is cleared (i.e. only when the actual file scan is performed) which would perhaps be too infrequent.

jeroen.b’s picture

+1 looking good, also great work on the test coverage.

I wonder if there's a way to make it fewer? But the only obvious way I can think of would be to show the message once after the cache is cleared (i.e. only when the actual file scan is performed) which would perhaps be too infrequent.

I believe that's what we had before. Only adding a log entry when the actual scan is performed.
It makes more sense to do that because mostly that's a point a developer/site builder will see it (on update.php or drush cc all), and afaik, that's the only one that is able to fix such issues.

stefan.r’s picture

Not a fan of only doing it on file scan, we want to incentivize people to fix their stuff, just a single log entry would be easily ignored... and missed on actual page loads on dev environments (which display trigger_error messages in a drupal_set_message)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/includes/bootstrap.inc
@@ -847,64 +854,253 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+  // Only write to the persistent cache if we are fully bootstrapped.
+  if (drupal_get_bootstrap_phase() != DRUPAL_BOOTSTRAP_FULL) {
+    return;
+  }

One question:

Should we use the "all_modules_loaded" condition here as that is what we really care about and not the bootstrap phase to be consistent?


Overall:

+2 to the new patch, RTBC

jeroen.b’s picture

Not a fan of only doing it on file scan, we want to incentivize people to fix their stuff, just a single log entry would be easily ignored... and missed on actual page loads on dev environments (which display trigger_error messages in a drupal_set_message)

I get that, but I wonder if showing it every time is going to help to do that. I'd rather go with a hook_requirements approach if we want to motivate people to fix it.
The current approach works fine for me (show on actual file scan), but maybe that's because I'm a CLI guy and I see the message clearly when I run drush updb.

stefan.r’s picture

@jeroen.b maybe we're actually on the same page -- I agree the current patch is fine. I think it does a trigger_error() on every failed drupal_get_filename call, rather than on every uncached file scan.

I remember we went through this earlier in the thread, the only real disadvantage is that we may be a bit too annoying in people's logs, but that's solved as soon as they fix the problem, and other PHP errors trigger on every request as well... So building some kind of limit in the error handler feels like unnecessary optimization and doing it only on uncached file scans feels like not doing it often enough. A hook_requirements() would be nice, but that can be looked at in #2464601: Check for missing modules in hook_requirements

Checked with @Fabianx as well and looks like we're good with the current patch:

3:19 PM <
> Fabianx-screen: for https://www.drupal.org/node/1081266 do you think it's OK to do the trigger_error() on every request? I don't really like doing it on file scan only
4:14 PM <
> stefan_r: I am okay with that.
David_Rothstein’s picture

One question:

Should we use the "all_modules_loaded" condition here as that is what we really care about and not the bootstrap phase to be consistent?

Actually, do we need to do either? What we're writing to the cache here is just the results of a filesystem scan, so why would it matter?

Instead of that, we could probably just check the #cache_merge_done key and make sure it's set, since that indicates that whatever came from the cache will get written back to the cache, and also ensures that the cache_set() call will succeed (in case this is done extremely early in the bootstrap) since it means cache_get() ran successfully earlier in the same request.

jeroen.b’s picture

@stefan.r, yeah, you're right. On every page request is fine.
Once every real filescan is way too easy to ignore/miss.

stefan.r’s picture

@David_Rothstein I guess now that we don't care about whether modules that are marked as missing/moved actually still are it may be fine to write to it always.

The only problem (which we already have anyway) is that it can be a bit confusing when debugging, and the "unnecessary" caching could lead to us cache a file scan before we want it to be cached in some edge cases (i.e. not finding the new location when a module is in both the profile and in sites/all), but that would be solved through a cache clear anyway.

Wouldn't this possibly be an extra unnecessary cache_set() call though - or the #cache_merge_done check would fix that?

David_Rothstein’s picture

Sorry, just to clarify, I didn't mean #cache_merge_done would be the only check (the #write_cache would still be checked too) so something like this:

function drupal_file_scan_write_cache() {
  // Only write to the persistent cache if requested, and if we know that any
  // data previously in the cache was successfully loaded and merged in by
  // _drupal_file_scan_cache().
  $file_scans = &_drupal_file_scan_cache();
  if (isset($file_scans['#write_cache']) && isset($file_scans['#cache_merge_done'])) {
    unset($file_scans['#write_cache']);
    cache_set('_drupal_file_scan_cache', $file_scans, 'cache_bootstrap');
  }
}
stefan.r’s picture

Yes I understood that much, just haven't given it any though yet - for now I don't see any big problem, so if you think this optimization is an improvement I have no objection.

OTOH if we leave the patch as-is, the only drawback is we don't write to persistent cache in the edge case of finishing the request before full bootstrap, doing a file scan that we currently do anyway, which I could live with as well, we already do a trigger_error() anyway.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
28.57 KB
PASSED: [[SimpleTest]]: [MySQL] 41,791 pass(es). View
1013 bytes

I could go either way also and don't have a huge preference. I think checking #cache_merge_done might be slightly nicer since it's more self-contained, so here's what the patch looks like if we change it to that.

Either way, we won't be able to get this patch in for this week's release unfortunately. Too many changes too recently, and I think we'll benefit from getting a bit more review here (and hopefully real world testing). I think we can definitely aim to get it in the next release though, which probably won't be too many months down the road either. Sorry for pushing back a lot on this issue, but since it's a big change that affects a very low-level part of Drupal, I think it needs extra review to make sure we get it right.

Fabianx’s picture

I agree with #546 that it is better to skip this release rather than rush it in. +1 to that.

The interdiff looks fine to me.

joseph.olstad’s picture

The new patch looks good, seems to be quick.

I have only a minor quirp, I'm a CLI guy and like using drush.

Patch #102 makes a nice pretty CLI output as follows:

WD system: The following module is missing from the file system: linkit. [error]

however Patch #546 outputs HTML onto the CLI in drush as follows:

PHP Warning: The following module is missing from the file system: <em class="placeholder">linkit</em>. In order to fix this, put the module back in its original location. For more information, see <a href="https://www.drupal.org/node/2487215">the documentation page</a>. in includes/bootstrap.inc on line 1079

However I'm fine with that as I'd rather have the PHP warning than no warning at all (but prefer the WD system message as in patch 102).

Either way it looks great! Thanks.

stefan.r’s picture

The CLI problem should rather be solved in drush IMO... this is not the only commands that outputs HTML

stefan.r’s picture

Anyone have #546 running on production or care to give this another review?

skadu’s picture

I'm not running #546 on production but just patched a development environment. All seems to be working fine for now. I will keep an eye on it over the next couple of weeks.

Only thing to mention was the patch output:

patching file includes/bootstrap.inc
patching file includes/common.inc
patching file includes/module.inc
patching file includes/update.inc
patching file modules/simpletest/simpletest.module
Hunk #1 succeeded at 374 (offset 3 lines).
patching file modules/simpletest/tests/bootstrap.test
patching file modules/system/system.updater.inc

Not sure if there is the offset because of something on my end or the patch, just wanted to share my findings.

  • catch committed b5f7ccc on 8.1.x
    Issue #1081266 by stefan.r, mikeytown2, jeroen.b, tsphethean, mfb,...
jeroen.b’s picture

I have been using #546 on a few sites for over a month now. No problems occurred.

jeroen.b’s picture

Actually, this generates an error when you want to install a profile.

The following module is missing from the file system: <em class="placeholder">{name-of-profile}</em>. In order to fix this, put the module back in its original location. For more information, see <a       [warning]
href="https://www.drupal.org/node/2487215">the documentation page</a>.

Line 1183 of includes/install.inc adds the profile name to the list of dependencies.
Line 1188 tries to load it as a module, which results in a error by _drupal_get_filename_fallback_trigger_error, as there is no module by the name of the profile.

jeroen.b’s picture

Status: Needs review » Needs work
stefan.r’s picture

@jeroen.b is loading a profile as if it were a module something we should support? I.e. should drupal_get_filename('module', 'profile_name') return the location of the profile?

jeroen.b’s picture

I'm not sure.
AFAIK drupal_get_filename has basic support for profiles by telling it you want a profile:

  if ($type == 'profile') {
    $profile_filename = "profiles/$name/$name.profile";
    $files[$type][$name] = file_exists($profile_filename) ? $profile_filename : FALSE;
  }

However, the profile is not a type "profile" in the system table, it's a "module" (which is kinda weird). So we should probably prime the cache somewhere as we do in D8.
drupal_get_filename('module', 'profile_name', 'profiles/profile_name/profile_name.profile');

stefan.r’s picture

I'm OK with that solution - it'd need a file_exists as well

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
29.12 KB
PASSED: [[SimpleTest]]: [MySQL] 41,781 pass(es). View
506 bytes

Here's a new patch. The file_exists is already in the code (4 lines above my addition). If it can't find the profile, the installation doesn't even start.

Just tested it, no error in install.

sylus’s picture

Hmm with the latest patch testing in Travis CI I am getting the following with Drush SI:

WD php: User warning: The following module is missing from the file  [warning]
system: . In order to fix this, put the module back in its original
location. For more information, see the documentation page. in
_drupal_get_filename_fallback_trigger_error() (line 1079 of
/home/travis/build/.../drupal-7/drupal/includes/bootstrap.inc).
stefan.r’s picture

@sylus could you may want to backtrace that - seems it's doing a drupal_get_filename('', 'module')?

@jeroen.b that fix seems fine...

sylus’s picture

Ah your right issue is related to #2383823: drupal_get_filename() should check if $name is empty I removed that patch as thought this patch alone would be enough, sorry for the noise :)

pwaterz’s picture

I'd like to make note of some what related issue that we were facing. At my current job we ran into an issue where we moved our site from dev/stage to production. The production environment sometimes has hicups and also sometimes can place modules in different directories that dev or stage. During some of the build hicups modules were not placed in the proper directories. We had issues where entire sets of modules would be auto disabled by drupal core. After a few hours of digging around I found that is was caused by update.php/drush updb. The update process calls https://api.drupal.org/api/drupal/includes%21update.inc/function/update_... to check 'compatibility' of modules and disables them if they aren't. One issue is that if a module is missing for some reason, like a broken symlink and you run drush updb, the system table gets updated and these modules get disabled. This is really scary for our organization because we run hundreds of drupal sites, and if this was to happen on massive scale, yikes! I think there should be a way to disable this feature in drupal core. Does this go along with this ticket or a new ticket?

stefan.r’s picture

@pwaterz new issue - it's unrelated to this one

stefan.r’s picture

Has anyone tested #546 or #559 yet?

jeroen.b’s picture

I have been using #559 since I posted the patch.
But it's my own patch so would be weird to put it to RTBC.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

I gave this another look and I concur with Fabianx in #547... David_Rothstein's changes still look great to me. joseph.olstad, sylus, skadu and jeroen.b have tested this in production, and #559 makes sense, so back to RTBC.

David_Rothstein’s picture

Agreed this look ready. Given the somewhat disruptive nature of this (and the late date compared to tomorrow's release window) I'm going to hold it for one more release; especially if we do #2598382: [policy] Adopt a 6-month feature release schedule for Drupal 7 (similar to Drupal 8) and use pseudo-semantic versioning this would be a great fit for an April 20 release of Drupal 7.50 which would have some other bigger changes also. But either way, rest assured it should be in the next non-security release after this one. Thanks everyone for sticking through a 550+ comment issue :)

I could not quite figure out #559 though. The change looks harmless but I can't figure out how to generate the problem (at least #546 seemed to work fine with the core install profiles).

@jeroen.b, what install profile were you using when you experienced that?

In particular:

Actually, this generates an error when you want to install a profile.
....
Line 1183 of includes/install.inc adds the profile name to the list of dependencies.
Line 1188 tries to load it as a module, which results in a error by _drupal_get_filename_fallback_trigger_error, as there is no module by the name of the profile.

I think for line 1183 you're referring to https://api.drupal.org/api/drupal/includes!install.inc/function/install_... but the code there is very explicit about making sure that the profile does not get listed as a dependency of itself. So it seems like it shouldn't ever try to load the .install file via the code you mentioned. That's why I'm not sure how to trigger this.

jeroen.b’s picture

@David_Rothstein
It's a custom profile. I just tested it but couldn't reproduce it with Drupal 7.41.
Then I remembered that specific profile used the Omega8 version, I was able to reproduce it with that.
I'll take it up with Omega8 when this patch lands. Patch #559 can be ignored.

kenorb’s picture

Shell command to test for missing modules:

while read -r file; do [ -f "$file" ] || echo "$file is missing."; done < <(drush sqlq "SELECT filename FROM system WHERE status = 1")

Fabianx’s picture

There is a problem with this patch (or rather sites that update to it):

It can under bad circumstances together with #2450533: SchemaCache is prone to persistent cache corruption / drupal_get_complete_schema() is not robust against recursion break a site:

From cold caches:

- hook_init() =>
- entity_get_info is called (by e.g. menu_set_custom_theme => get_admin_paths() => entity_translation_admin_paths() => bean => ... )
- That triggers drupal_get_schema()
- That triggers drupal_get_complete_schema()
- That calls field_sql_storage_schema, which checks all fields for data
- This calls drupal_get_filename() to load the .install files

- This leads to watchdog being triggered. (Note: The module is just referenced somewhere, but not even in the system table anymore).

- rules_watchdog triggers a loading of event rules
- That triggers ctools_load_plugins => ... => entity_get_info()
- That triggers drupal_get_complete_schema() again, which returns the now incompletely build schema()
- That leads to errors and entity_get_info() being incompletely populated.

While not the fault of this patch, this condition is only resolvable by putting the original module back (or an empty .install file) as it is very hard to find and prune all data referencing somewhere to a module file.

On the other hand, without this patch a system scan would always be done, which would lead to strange and unexplainable slow down and the warning message gives a dev at least a chance to restore the modules.

However a watchdog call can have side effects during early bootstrap or during cache rebuilding.

So my proposal:

'Cache' the watchdog call in a static variable and execute it during shutdown (drupal_exit()), where we also create the caches for e.g. modules. That reduces / eliminates the side effects of this. There is a slight chance that a site crashes and drupal_exit() is not called. However I think we need to live with that.

The 'bad' cache is still good and solves the performance problems to avoid re-scanning the filesystem.

@David: What do you think?

stefan.r’s picture

So, postpone execution of any and all watchdog calls that happen during early bootstrap to the end of the request?

Should we discuss in a separate issue?

Fabianx’s picture

#572: No, not all watchdog calls, that would indeed be a follow-up. Just this newly introduced one for now.

e.g.


$watchdog_collector = &drupal_static('drupal_watchdog_collector', array());
$watchdog_collector[] = array('Some Message', WATCHDOG_INFO);

// error_log should not call any hooks, but log to a file / null or syslog. That should ensure in case of a crash the message can be found somewhere.
error_log('Warning: Some Message');

function hook_exit() {
  $watchdog_collector = &drupal_static('drupal_watchdog_collector', array());
  if (!empty($watchdog_collector)) {
    foreach ($watchdog_collector as $watchdog_entry) {
      call_user_func_array('watchdog', $watchdog_entry);
    }
  }
}
David_Rothstein’s picture

Hm, I think that makes sense, but the patch is currently using trigger_error (rather than a direct watchdog call) in order to get the message to appear on the screen on development sites... I'm not sure offhand what happens if you try to do a trigger_error that late (after sessions have already been committed).

We could also look at trying to get #2450533: SchemaCache is prone to persistent cache corruption / drupal_get_complete_schema() is not robust against recursion fixed in the same release. (At this point I think we're aiming for May 4 rather than April 20.) But I think you're saying that you see other potential risks with this watchdog call being directly triggered by a low-level function?

It looks like rules_watchdog() is doing some pretty heavy stuff for a low-level hook like hook_watchdog(); ideally it would not do that, but it is what it is :)

Fabianx’s picture

#574: Indeed, my distribution site used an older version of this patch, which is why I was confused.

If we only trigger_error(), we should be okay. I will confirm on my site, but this means the patch can go ahead as planned.

And I agree that rules_watchdog() should at least also check for BOOTSTRAP_FULL or at least their (rules) hook_init() should be moved as last item in the hook chain. I will open an issue.

marcelovani’s picture

I think this is related to https://www.drupal.org/node/2324587#comment-9397273
Can you check if you are using the version of Rules that contains the fix of patch #37?

stefan.r’s picture

So I guess this will be for a May 4 release?

Just a note that we'll need to update the docs at https://www.drupal.org/node/2487215

SocialNicheGuru’s picture

I applied the patch (awesome patch by the way).
I deleted a directory by accident
I got this on my screen
Notice: Undefined variable: trigger_error in drupal_get_filename() (line 888 of /includes/bootstrap.inc).
the line is:

$files[$type][$name] = _drupal_get_filename_fallback($type, $name, $trigger_error, $database_unavailable);

should we pass in 'TRUE' for $trigger_error since the variable is not defined and as no default value?

stefan.r’s picture

#578: I think we already do? You'll want to use the patch in #559 and ensure it is applied correctly, I don't see how you would get an "undefined variable" error like that when the function signature says:

function drupal_get_filename($type, $name, $filename = NULL, $trigger_error = TRUE) {
alexmoreno’s picture

#559 tested (with an Omega subtheme, although don't think it's relevant) and works more than fine.

I was doing some profiling and found an alarming file_scan_directory function being called more than 1.000 times per call. Applying the patch makes this to dissapear, although I'd recommend anyone to dig further and remove any entries or modules disabled but not uninstalled and similar.

IMO this should be merged ASAP, as it is relatively easy to reproduce in a big/medium site with a big team working for some time (you know, one disables a module, someone else enables, ... you get the idea).

stefan.r’s picture

@Fabianx re #575, doesn't trigger_error() cause a watchdog call?

stefan.r’s picture

Status: Reviewed & tested by the community » Needs work

Looking at this with jeroen.b, back to NW for #571

jeroen.b’s picture

Status: Needs work » Needs review
Issue tags: -D8 Accelerate Dev Days +drupaldevdays
FileSize
30 KB

After discussion with @stefan.r we decided to only delay the trigger_errors for the missing/moved errors so we don't have to touch other code base. We are storing the errors in a drupal_static and then doing a trigger_error in _drupal_bootstrap_full. We are not doing it in system_exit because it's too late to do an drupal_set_message so then the developer wouldn't see the error message.

jeroen.b’s picture

FileSize
3.95 KB

Whoops, missed the interdiff.

Status: Needs review » Needs work

The last submitted patch, 583: 1081266-582.patch, failed testing.

jeroen.b’s picture

Ah, yeah, that makes sense.
Because we do the invoke in _drupal_bootstrap_full(), all the missing/moved errors after _drupal_bootstrap_full won't do a trigger_error.

My solution would be to set another drupal_static in _drupal_get_filename_fallback_trigger_error_invoke whether the invocation has already happened.
In _drupal_get_filename_fallback_trigger_error we would check for that static. The logic to do the actual trigger error would be separated in a new function. When the static is TRUE, the trigger_error happens directly, if it is FALSE the trigger_error is delayed until bootstrap_full.

jeroen.b’s picture

Status: Needs work » Needs review
FileSize
31.01 KB
5.33 KB
4.45 KB

Here's a patch that does what I described.

Status: Needs review » Needs work

The last submitted patch, 587: 1081266-587.patch, failed testing.

jeroen.b’s picture

We might need to update the bootstrap test a bit. It looks directly for a triggered error in the custom error handler, but since we delay the messages now it will fail.

stefan.r’s picture

Issue tags: +Drupal 7.50 target
Fabianx’s picture

I like the idea, I am a little worried about the implementation.

For me this should just be:

function trigger_error_safe(string $error_msg , int $error_type = E_USER_NOTICE) {

  if (drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL) {
    trigger_error($error_msg, $error_type);
    return;
  }

  // It is not yet safe to trigger an error, so queue it up.

  $errors = &drupal_static(__FUNCTION__, array());
  $errors[] = array($error_msg, $error_type);
}

function trigger_all_queued_errors() {
  $errors = &drupal_static('trigger_error_queued', array());

  if (empty($errors)) {
    return;
  }
  foreach ($errors as $error_parameters) {
     call_user_func_array('trigger_error', $error_parameters);
  }
}

Possibly can add something to the message as done before, but the implementation should really be a drop-in replacement for trigger_error(), which we call after we have completed the bootstrap_phase().

jeroen.b’s picture

Thanks Fabian. I get your feedback.
However, after some thinking, shouldn't we just make trigger_error "phase safe"?

Fabianx’s picture

#592: trigger_error() is an internal function, so we can't override it.

On the other hand I would not like to make our error handler depend on bootstrap phase.

So lets please do it the way I suggest in #591.

jeroen.b’s picture

That's not what I meant. I think we should fix it inside our own error handler instead of creating a trigger_error_safe() because we might not be the only one possibly calling trigger_error too early.

Fabianx’s picture

#594: And that is an excellent idea, but not feasible to put into this is already large patch.

Lets use _drupal_trigger_error_safe and _drupal_trigger_all_queued_errors as names then we can technically still remove the functionality again or re-use it from our error handler.

Reasoning:

- We add a new trigger_error() call, where there was none before
- Therefore we get new side effects from this trigger_error() call - e.g. my updb dieing
- If we remove the side effects for _just_ this call, we are good to go for the run-time
- This only means trigger_error() is called shortly bootstrap full phase is reached, which is an acceptable stage to trigger an error

- If we introduce queueing of errors in the error handler now, we get new side effects related to that
- This might lead to unexpected behavior on other sites, etc.

Therefore - even though the idea is great - and even though it would be better to fix this at the "core" in the error handler, this is not helping this patch to get into 7.50 any time soon.

Isolation of side effects needs to be our goal to get this patch in - if we want it now.

jeroen.b’s picture

I get your reasoning. I would be fine with _drupal_trigger_error_safe just for the sake of getting it in.
Maybe we should create a follow-up issue for fixing the watchdog call inside trigger_error?

stefan.r’s picture

Yes, sounds like follow-up material :)

Fabianx’s picture

#596: Yes, a follow-up would be great!

jeroen.b’s picture

@Fabianx will you create the new patch or should I?

Fabianx’s picture

#559: Can you do that please? I am pretty busy atm.

David_Rothstein’s picture

The above plan sounds good to me, but I don't understand this part from #591:

  if (drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL) {
    trigger_error($error_msg, $error_type);
    return;
  }

The main scenario we're trying to protect against here (from #571) is triggered in hook_init() (during full bootstrap) so won't the above code fail to protect against it? (As far as I know, drupal_get_bootstrap_phase() returns DRUPAL_BOOTSTRAP_FULL anytime after the full bootstrap phase has begun.)

But if we leave that code out (which would make the function's behavior more consistent/predictable anyway) then it makes sense to me.

I'd also consider calling the new functions _drupal_trigger_error_delayed() and _drupal_trigger_all_delayed_errors() (or similar - the key point being to use the same terminology for each).

jeroen.b’s picture

@David_Rothstein You are right about the naming of the functions, we should change it.
You are also right about drupal_get_bootstrap_phase() returning DRUPAL_BOOTSTRAP_FULL when it started bootstrapping, not when it finished bootstrapping.
That's actually inconsistent with documentation, as it says "The current phase is the one most recently completed by drupal_bootstrap()".

Thing is, we can't just remove these lines:

if (drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL) {
    trigger_error($error_msg, $error_type);
    return;
  }

Because then we will have no trigger_error() when Drupal has been fully bootstrapped as _drupal_trigger_all_queued_errors() will never be executed.

jeroen.b’s picture

Here is the new patch for completeness. Like @David_Rothstein said it will probably not work.
I could also not think of a proper way to test this edge case.

stefan.r’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 603: 1081266-600.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
29.72 KB
3.24 KB

Ah, I see.

Well it's kind of ugly but it could be possible to do something like the attached (rough code, untested).

It does seem to me like the key point here is not what stage of the bootstrap you're in, but whether or not you're potentially in the middle of an operation that can't be "interrupted". Because calling trigger_error() => watchdog() at any point in time will invoke a hook which runs arbitrary code which could involve something that breaks the operation you're in the middle of. I think that's effectively what we're trying to say here.

Note this patch is built on top of #546 (not #559) since I think we agreed in #568/#569 that the change in #559 wasn't necessary.

jeroen.b’s picture

That would probably work yeah. Any idea how we can create a test for this?

stefan.r’s picture

I think that may be fine, for the test we can check the watchdog table after a call to _drupal_trigger_error_with_delayed_logging() (if needed, from a test module, for example with a drupalGet() to a page that triggers an error)

stefan.r’s picture

This needs the @todo documented and ideally a test

David_Rothstein’s picture

I should be able to look into that a little later tonight.

David_Rothstein’s picture

Here is a new patch with the documentation plus additional tests.

In addition to testing the watchdog() call itself, I think I was able to test the actual scenario with drupal_get_schema() failing. It's a bit convoluted, but still may have been easier than setting up the Rules module in such a way as to trigger this as per #571 and then testing that manually :)

I'm also posting a patch with the new tests combined with the earlier patch from this issue. If this is working correctly we should see failures for some of the new tests in that case.

David_Rothstein’s picture

The interdiff is compared to #606, by the way.

The last submitted patch, 611: 1081266-546-plus-new-tests-should-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 611: 1081266-611.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review

Retesting - that looks like a random testbot fail on the real patch.

The first patch failed with the expected failure.

stefan.r’s picture

Assigned: Unassigned » Fabianx
Status: Needs review » Reviewed & tested by the community

Interdiff looks great, with some very nice tests!

Fabianx’s picture

The new solution to just trigger the watchdog call delayed is great!

Giving credit to a lot of people on this issue (soooo many have tested the patch or given otherwise useful feedback).

Marking for commit.

  • Fabianx committed 206c7c1 on 7.x
    Issue #1081266 by stefan.r, jeroen.b, mikeytown2, David_Rothstein,...
Fabianx’s picture

Assigned: Fabianx » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit +7.50 release notes

Committed and pushed to 7.x! Thanks all!

Fixed on commit (as git had a warning related to whitespace problems being added):

diff --git a/modules/simpletest/tests/system_test.install b/modules/simpletest/tests/system_test.install
index 18d4af9..c209233 100644
--- a/modules/simpletest/tests/system_test.install
+++ b/modules/simpletest/tests/system_test.install
@@ -18,5 +18,3 @@ function system_test_schema() {
 
   return array();
 }
-
-
David_Rothstein’s picture

Thanks for going through and figuring out all those issue credits - that must have been a lot of work :)

I edited the change notice to make it a bit more relevant for site builders (who might see this PHP warning too), and published it:
https://www.drupal.org/node/2581445/revisions/view/8963663/9848351

I also edited the documentation page to indicate this is now in Drupal 7.50:
https://www.drupal.org/node/2487215/revisions/view/9220114/9848355

We are hopefully finally done with this enormously long issue. Thanks everyone!

DamienMcKenna’s picture

FYI I'm seeing a weird regression in Metatag's tests from this change: #2761829: Getting drupal_trigger_error_with_delayed_logging errors on Drupal 7.x dev (7.50)

MustangGB’s picture

43 "missing" modules :'(

MustangGB’s picture

Serious question now though, I have a "profiles/default/default.profile" entry in my system table, was there ever a hook_update_N() that should have removed this?

joseph.olstad’s picture

@MustangGB , for those missing modules, if you can, put them back into the filesystem temporarily, then run a pm_uninstall either
drush pmu module_was_awol module_was_awol2 module_was_awol3 module_was_awol4 module_was_awol5 module_was_awol6 module_was_awol7 module_was_awolXX module_was_awol40 -y

or, (if all else fails) in the case that you cannot find the module(s) install this cleanup utility (for use with drush) or for some reason the uninstall fails:

download this ZIP of cleanup_tool
extract it to your sites/all/modules folder

USAGE:

drush en cleanup_tool -y;
drush cc drush;
drush cu-check;
drush cu-clean all --update_option=all -y;
drush dis cleanup_tool -y;
drush pml | grep cleanup_tool;

For more details, please see project page

MustangGB’s picture

@joseph.olstad:

Yea don't worry I've already sorted them all out, except for "default".

My question was really regarding people that have upgraded from D5/D6 and whether "default" should have already been removed during the upgrade/migration process, and are there any consequences of it not having been dealt with, or should we just remove it now with a new hook_update_N().

(Nice module though)

stefan.r’s picture

@MustangGB can you open a new issue for the default.profile problem, in case we want to fix this in an update hook?

MustangGB’s picture

charginghawk’s picture

FYI, the D8 fix spawned #2486083: The following module is missing from the file system: standard, an issue which seems to be appearing in 7.50. We may need to port the fix there to D7.

axel.rutz’s picture

Thanks everyone. A 40k patch, finally awesone!

Status: Fixed » Closed (fixed)

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

Pages