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

[woutersamaey]

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 (*)

1. 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.
2. Refresh the page. The output will be cut off and you will NOT see any errors.

Contribution checklist (*)

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

[luigifab]

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]

@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]

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]

I added a new testcase for triggering the autoloader for a non existing class https://github.com/OpenMage/Testfield/commit/ef1a4aee6ef402763d80fa216e3d4b616dddbb89

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]

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

[Flyingmana]

@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]

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

[tmotyl]

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]

@tmotyl my thoughts exactly...

[luigifab]

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]

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]

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]

This is a good thing, right?

[luigifab]

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]

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]

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.

https://github.com/OpenMage/magento-lts/blob/f09b66403a7991f52649337c6053c987c2ffbed6/app/Mage.php#L50-L55

https://github.com/OpenMage/magento-lts/blob/f09b66403a7991f52649337c6053c987c2ffbed6/lib/phpseclib/bootstrap.php#L17-L22

[luigifab]

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]

What are we doing with this one guys?

[luigifab]

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]

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]

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