Stop hiding errors inside broken PHP files when loaded through the autoloader #1817

woutersamaey · 2021-09-09T12:06:12Z

Description (*)

A PHP class of mine had by mistake a method return type of : true instead of :bool making the PHP file unparsable.
The Magento autoloader suppresses all errors, so I could not see this error at all.
Because of this, I lost many hours trying to find the error.

I believe Magento should not suppress any errors when loading files. We definately want to know about these.

Manual testing scenarios (*)

Purposefully break a PHP file that would be loaded through the autoloader. In my case I added a method return type : true instead of : bool.
Refresh the page. The output will be cut off and you will NOT see any errors.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All automated tests passed successfully (all builds are green)
Add yourself to contributors list

luigifab · 2021-09-09T12:27:48Z

Without the @, you didn't get Warning: include(Phpseclib\Crypt\Base.php): Failed to open stream: No such file or directory in lib/Varien/Autoload.php on line 65? (Debian, PHP 8.0.10, OpenMage 20.0.13)

woutersamaey · 2021-09-10T06:45:23Z

@luigifab I have not encountered this issue so far. It does look like you have this issue because of case sensitivity on your file system. If this is the case, we should fix the directory and/or filename as it is clearly wrong. This would also mean that Phpseclib is not working on your end as the file cannot be loaded and you just didn't know about it.

I cannot think of any good reason why we would want to hide errors during PHP file loading. Whatever the reason for the @include was, sounds like there are better ways to deal with specific issues.

luigifab · 2021-09-23T18:26:53Z

On our installation, we have already created a symlink from phpseclib to Phpseclib to avoid an error (I don't remember why).

But, for my warning, I getting it when I go to System / Configuration, when a field frontend_type is obscure.
My installation is located on a btrfs filesystem with defaults,discard,relatime options.

Flyingmana · 2021-09-28T18:52:52Z

I added a new testcase for triggering the autoloader for a non existing class
OpenMage/Testfield@ef1a4ae

this test currently fails with this change. (which is the actual reason, why we added the @ there initially)

But the issue you want to solve is definitely valid, too. We just need to find a solution which works for both cases.

woutersamaey · 2021-09-28T20:40:34Z

I don’t follow. You need the “@“ for a test case?

Flyingmana · 2021-09-28T21:04:35Z

@woutersamaey no, the testcase imitates a common usecase.

in other words: remove the @, and you break at least every M1 project I ever worked on

woutersamaey · 2021-09-29T06:48:39Z

@Flyingmana I still don't follow. You have projects that load missing classes?

tmotyl · 2021-09-29T07:59:55Z

Are you saying that openmage core tries to load classes which doesn't exist? or that extensions/custom modules are doing so?
instead of @ operator we could check if file exists before autoloading and maybe log error to PHP error log when class doesn't exist? or just do it in dev mode?

woutersamaey · 2021-09-29T08:02:38Z

@tmotyl my thoughts exactly...

luigifab · 2021-09-30T18:50:25Z

We have tried the removal of the @ [local (dev mode) and test (no dev mode) environments] with the #1780 fix.
No error for now.

Flyingmana · 2021-09-30T20:42:54Z

the testcase is pretty clear

    public function testClassDoesNotExists()
    {
        $this->assertFalse(class_exists('Mage_Non_Existent'));
    }

using a class_exist() on a class which could not exist is a very normal thing in php.
It should not cause an error, if the class does not exist.

I just want to make sure, this still works after the change.

luigifab · 2021-12-14T13:40:52Z

In any phtml:

<?php class_exists('Mage_Non_Existent') ?>

Result in:

Warning: include(Mage/Non/Existent.php): failed to open stream: No such file or directory
 in lib/Varien/Autoload.php on line 66

woutersamaey · 2021-12-14T14:52:29Z

This is a good thing, right?

luigifab · 2021-12-14T15:07:03Z

I think if I try if (class_exists('Mage_Non_Existent')), I would like true or false, not an error because the file does not exist.
But if there is an error inside Mage_Non_Existent, I would like an error.
No?

Asfolny · 2021-12-15T12:51:06Z

the testcase is pretty clear
    public function testClassDoesNotExists()
    {
        $this->assertFalse(class_exists('Mage_Non_Existent'));
    }
using a class_exist() on a class which could not exist is a very normal thing in php. It should not cause an error, if the class does not exist.

I just want to make sure, this still works after the change.

Out of curiosity, but a class that could not exist in an if class_exists, isn't that just an if false?

On another note, a warning on class_exists when a class doesn't exist doesn't seem good.

What if we did the following instead of ignoring all issues with an @?

$filepath = str_replace(' ', DIRECTORY_SEPARATOR, ucwords(str_replace('_', ' ', $class))) . '.php';
foreach (explode(PATH_SEPARATOR, get_include_path()) as $candidatePath) {
    if (file_exists($candidatePath . DIRECTORY_SEPARATOR . $filepath)) {
        return include $filepath;
    }
}

EDIT: updated suggestion to actually work.
Turns out I didn't actually know about the include_path and how magento 1 uses it, I have been spoiled by composer...

fl1pfl0p · 2022-01-31T12:27:13Z

Are you saying that openmage core tries to load classes which doesn't exist? or that extensions/custom modules are doing so? instead of @ operator we could check if file exists before autoloading and maybe log error to PHP error log when class doesn't exist? or just do it in dev mode?

Although there is an warning from the openmage autoloader when trying to load Phpseclib (suppressed by the @) there is a second autoloader registered that takes care of the loading of Phpseclib. Reordering the autoloader registrations or modifying the openmage autoloader to handle Phpseclib better should be considered to avoid any warnings.

magento-lts/app/Mage.php

Lines 50 to 55 in f09b664

    
           include_once "Varien/Autoload.php"; 
        
           Varien_Autoload::register(); 
        
           include_once "phpseclib/bootstrap.php"; 
        
           include_once "mcryptcompat/mcrypt.php";

magento-lts/lib/phpseclib/bootstrap.php

Lines 17 to 22 in f09b664

    
           spl_autoload_register(function($className) { 
        
               $parts = explode('\\', $className); 
        
               if (array_shift($parts) == 'phpseclib') { 
        
                   return include str_replace('\\', DIRECTORY_SEPARATOR, $className) . '.php'; 
        
               } 
        
           });

luigifab · 2022-02-02T13:03:48Z

By loading phpseclib/bootstrap.php before Varien/Autoload.php, my error (#1780) is fixed.
At the same time by fixing Varien Autoloader, we can remove spl_autoload_register of phpseclib, no?

fballiano · 2022-05-02T12:44:02Z

What are we doing with this one guys?

luigifab · 2022-05-02T19:18:18Z

I vote for that, but I forgot if it works very well or not, and not deployed in production:

diff --git a/app/Mage.php b/app/Mage.php
index 910cb4804..615c2aabb 100644
--- a/app/Mage.php
+++ b/app/Mage.php
@@ -47,11 +47,11 @@ $paths[] = BP . DS . 'lib';
 $appPath = implode(PS, $paths);
 set_include_path($appPath . PS . Mage::registry('original_include_path'));
 include_once "Mage/Core/functions.php";
+include_once "phpseclib/bootstrap.php";
 include_once "Varien/Autoload.php";
 
 Varien_Autoload::register();
 
-include_once "phpseclib/bootstrap.php";
 include_once "mcryptcompat/mcrypt.php";
 
 /* Support additional includes, such as composer's vendor/autoload.php files */
diff --git a/lib/Varien/Autoload.php b/lib/Varien/Autoload.php
index bfaa67054..479948527 100644
--- a/lib/Varien/Autoload.php
+++ b/lib/Varien/Autoload.php
@@ -62,6 +62,6 @@ class Varien_Autoload
      */
     public function autoload($class)
     {
-        return @include str_replace(' ', DIRECTORY_SEPARATOR, ucwords(str_replace('_', ' ', $class))) . '.php';
+        return include str_replace(' ', DIRECTORY_SEPARATOR, ucwords(str_replace(['_', '\\'], [' ', DIRECTORY_SEPARATOR], $class))) . '.php';
     }
 }
diff --git a/lib/phpseclib/bootstrap.php b/lib/phpseclib/bootstrap.php
index 55a919e89..f3c2d2e06 100644
--- a/lib/phpseclib/bootstrap.php
+++ b/lib/phpseclib/bootstrap.php
@@ -1,9 +1,9 @@
 <?php
 /**
  * Bootstrapping File for phpseclib
- *
  * @license http://www.opensource.org/licenses/mit-license.html MIT License
  */
+
 if (extension_loaded('mbstring')) {
     // 2 - MB_OVERLOAD_STRING
     if (ini_get('mbstring.func_overload') & 2) {
@@ -13,10 +13,3 @@ if (extension_loaded('mbstring')) {
         );
     }
 }
-
-spl_autoload_register(function($className) {
-    $parts = explode('\\', $className);
-    if (array_shift($parts) == 'phpseclib') {
-        return include str_replace('\\', DIRECTORY_SEPARATOR, $className) . '.php';
-    }
-});

Here, I missed Asfolny proposal.

tmotyl · 2022-05-03T11:01:11Z

Im in favor of making the code not hide errors. In general i would be also in favor of moving the autoloading out of the Mage class. The current mixture of executable code and class declaration is a pain eg when using tools like phpstan. But this change is of course a separate topic/pr

Flyingmana · 2022-07-10T09:15:11Z

I vote for that, but I forgot if it works very well or not, and not deployed in production:

diff --git a/app/Mage.php b/app/Mage.php
index 910cb4804..615c2aabb 100644
--- a/app/Mage.php
+++ b/app/Mage.php
@@ -47,11 +47,11 @@ $paths[] = BP . DS . 'lib';
 $appPath = implode(PS, $paths);
 set_include_path($appPath . PS . Mage::registry('original_include_path'));
 include_once "Mage/Core/functions.php";
+include_once "phpseclib/bootstrap.php";
 include_once "Varien/Autoload.php";
 
 Varien_Autoload::register();
 
-include_once "phpseclib/bootstrap.php";
 include_once "mcryptcompat/mcrypt.php";
 
 /* Support additional includes, such as composer's vendor/autoload.php files */
diff --git a/lib/Varien/Autoload.php b/lib/Varien/Autoload.php
index bfaa67054..479948527 100644
--- a/lib/Varien/Autoload.php
+++ b/lib/Varien/Autoload.php
@@ -62,6 +62,6 @@ class Varien_Autoload
      */
     public function autoload($class)
     {
-        return @include str_replace(' ', DIRECTORY_SEPARATOR, ucwords(str_replace('_', ' ', $class))) . '.php';
+        return include str_replace(' ', DIRECTORY_SEPARATOR, ucwords(str_replace(['_', '\\'], [' ', DIRECTORY_SEPARATOR], $class))) . '.php';
     }
 }
diff --git a/lib/phpseclib/bootstrap.php b/lib/phpseclib/bootstrap.php
index 55a919e89..f3c2d2e06 100644
--- a/lib/phpseclib/bootstrap.php
+++ b/lib/phpseclib/bootstrap.php
@@ -1,9 +1,9 @@
 <?php
 /**
  * Bootstrapping File for phpseclib
- *
  * @license http://www.opensource.org/licenses/mit-license.html MIT License
  */
+
 if (extension_loaded('mbstring')) {
     // 2 - MB_OVERLOAD_STRING
     if (ini_get('mbstring.func_overload') & 2) {
@@ -13,10 +13,3 @@ if (extension_loaded('mbstring')) {
         );
     }
 }
-
-spl_autoload_register(function($className) {
-    $parts = explode('\\', $className);
-    if (array_shift($parts) == 'phpseclib') {
-        return include str_replace('\\', DIRECTORY_SEPARATOR, $className) . '.php';
-    }
-});

Here, I missed Asfolny proposal.

this will still cause problems, if the class does not exist, because include throws I think a notice, which then is transformed into an exception

Stop hiding errors inside broken PHP files

f65db57

github-actions bot added Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* labels Sep 9, 2021

Flyingmana mentioned this pull request Jul 10, 2022

autoload without hiding errors #2300

Merged

4 tasks

luigifab mentioned this pull request Nov 17, 2022

Rewrite autoloader #2734

Closed

4 tasks

fballiano closed this in #2300 Dec 25, 2022

Uh oh!

Stop hiding errors inside broken PHP files when loaded through the autoloader #1817

Stop hiding errors inside broken PHP files when loaded through the autoloader #1817

Uh oh!

Conversation

woutersamaey commented Sep 9, 2021

Description (*)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

luigifab commented Sep 9, 2021

Uh oh!

woutersamaey commented Sep 10, 2021

Uh oh!

luigifab commented Sep 23, 2021

Uh oh!

Flyingmana commented Sep 28, 2021

Uh oh!

woutersamaey commented Sep 28, 2021

Uh oh!

Flyingmana commented Sep 28, 2021

Uh oh!

woutersamaey commented Sep 29, 2021

Uh oh!

tmotyl commented Sep 29, 2021

Uh oh!

woutersamaey commented Sep 29, 2021

Uh oh!

luigifab commented Sep 30, 2021

Uh oh!

Flyingmana commented Sep 30, 2021

Uh oh!

luigifab commented Dec 14, 2021

Uh oh!

woutersamaey commented Dec 14, 2021

Uh oh!

luigifab commented Dec 14, 2021

Uh oh!

Asfolny commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fl1pfl0p commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luigifab commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fballiano commented May 2, 2022

Uh oh!

luigifab commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmotyl commented May 3, 2022

Uh oh!

Flyingmana commented Jul 10, 2022

Uh oh!

Uh oh!

Asfolny commented Dec 15, 2021 •

edited

Loading

fl1pfl0p commented Jan 31, 2022 •

edited

Loading

luigifab commented Feb 2, 2022 •

edited

Loading

luigifab commented May 2, 2022 •

edited

Loading