Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Suggested fix for issue #4385 #6455

Closed
wants to merge 3 commits into from
Closed

Suggested fix for issue #4385 #6455

wants to merge 3 commits into from

Conversation

emeraldinspirations
Copy link
Contributor

WHEREAS: major revisions in PHP indicate breaking changes that
sometimes render code from older versions uncompilable AND

WHEREAS: uncompilable changes can not be resolved by if...then
statements, requiring (at times) completely different files for
different PHP versions AND

WHEREAS: a PHP developer may need to maintain code on multiple PHP
platform versions simultaneously AND

WHEREAS: in the Wikipedia article about PHP
(https://en.wikipedia.org/wiki/PHP) there is reference to version based
PHP file extensions (.php3, .php4, .php5, .php7). (
Unfortunately there is no reference in the document as to the source of
this data.);

THEREFORE I SUGGEST: an additional if...then condition in the
autoloader that searches for the respective versioned extension
(.php3, .php4, .php5, .php7) when the default extension
(.php) is not found.

This would allow seamless deployment across PHP major versions.

This also prevents the developer from having to maintain multiple
copies of their entire codebase (either by having multiple independent
namespaces or VCS trees). Only files that required different versions
would use the different versioned extensions.

Example:

- testClassA.php
- testClassB.php5
- testClassB.php7
- testClassC.php

The developer could also use the same unit tests across versions. A
developer simply runs the unit tests in the newest version, then runs
the same library of tests on the older platform (ex. via VM).

Dropping support for an older platform would also be much simpler,
involving a fairly simple script.

WHEREAS: major revisions in PHP indicate breaking changes that
sometimes render code from older versions uncompilable AND

WHEREAS: uncompilable changes can not be resolved by if...then
statements, requiring (at times) completely different files for
different PHP versions AND

WHEREAS: a PHP developer may need to maintain code on multiple PHP
platform versions simultaneously AND

WHEREAS: in the Wikipedia article about PHP
(https://en.wikipedia.org/wiki/PHP) there is reference to version based
PHP file extensions (`.php3`, `.php4`, `.php5`, `.php7`).  (
Unfortunately there is no reference in the document as to the source of
this data.);

THEREFORE I SUGGEST: an additional if...then condition in the
autoloader that searches for the respective versioned extension
(`.php3`, `.php4`, `.php5`, `.php7`) when the default extension
(`.php`) is not found.

This would allow seamless deployment across PHP major versions.

This also prevents the developer from having to maintain multiple
copies of their entire codebase (either by having multiple independent
namespaces or VCS trees).  Only files that required different versions
would use the different versioned extensions.

Example:
```
- testClassA.php
- testClassB.php5
- testClassB.php7
- testClassC.php
```

The developer could also use the same unit tests across versions.  A
developer simply runs the unit tests in the newest version, then runs
the same library of tests on the older platform (ex. via VM).

Dropping support for an older platform would also be much simpler,
involving a fairly simple script.
```
PHP Parse error:  syntax error, unexpected '[' in
/home/travis/build/composer/composer/vendor/composer/ClassLoader.php on
line 358
```

Replaced bracketed element reference with `array_shift`
@alcohol
Copy link
Member

alcohol commented May 29, 2017

May I suggest using PHP_MAJOR_VERSION instead? This is available since 5.2.7 which already meets the minimum required PHP version of Composer itself.

@emeraldinspirations
Copy link
Contributor Author

Thank you @alcohol. I did not know which version PHP_MAJOR_VERSION was released, nor how far back Composer's minimum was. Should have researched it.

@alcohol
Copy link
Member

alcohol commented May 29, 2017

I think the performance impact could be considered negligible considering it would only be a follow-up search for misses. @Seldaek what do you think?

@Seldaek
Copy link
Member

Seldaek commented Jun 15, 2017

I am rather strongly against this change. The impact is hard to judge because people might chain autoloaders and suddenly all classes going through the composer one would then take twice as long to load, that's not really acceptable. In the classmap generator we do support .inc for silly BC reasons already, and .php.. but really since the early php5 days where people thought it was a good idea to use .php5, I haven't seen this used anywhere in the wild. I am not keen at all on adding support for more of those if there is virtually zero demand.

Besides, I don't really agree with your 2nd WHEREAS assertion, do you have an actual example of some code that is not possible to make work against multiple versions? Because basically a whole ecosystem of libraries works fine on the whole 5.3-7.1 range, including Composer, Symfony, etc.

@emeraldinspirations
Copy link
Contributor Author

@Seldaek, I agree with your concern about chaining Autoloaders. Being new to gitHub and open source projects, I am hesitant to suggest larger refactoring projects until I have more experience.

My first instinct when I saw this "chaining" was to put each of the existing "autoloaders" into their own classes implementing some sort of AutoloaderInterface. This would have several benefits like:

  • Increasing the compliance with the open/close principle
  • Cleaning up the code (see example below)
  • Only running the necessary autoloaders (per some optional setting in the config file)
  • Allowing developers to create autoloader plugins that could be included as needed
  • Decoupling the code allowing proper unit testing for each of the autoloaders
/**
 * Finds the path to the file where the class is defined.
 *
 * @param string $class                The name of the class
 * @param array  $autoloaderClassArray An array of names of classes that
 *                                     implement AutoloaderInterface
 *
 * @todo Note: This code has not been tested, it is an example
 *
 * @return string|false The path if found, false otherwise
 */
public function findFile(string $class, array $autoloaderClassArray = null)
{
    // work around for PHP 5.3.0 - 5.3.2 https://bugs.php.net/50731
    if ('\\' == $class[0]) {
        $class = substr($class, 1);
    }

    // If not specified, pull from config file or default values
    $autoloaderClassArray 
        = $autoloaderClassArray
        ?? ConfigFile::getAutoloaderClassArray();
    // Example (current default): [
    //     'ClassMapLookup',
    //     'MissingClassLookup',
    //     'DefaultPhpFileAutoloader',
    //     'HHVMFileAutoloader';
    // ];
    
    $return = false;
    
    foreach ($autoloaderClassArray as $autoloaderClassName) {
        $autoloader = new $autoloaderClassName();
        if ($autoloader instanceof AutoloaderInterface) {
            $return = $autoloader->findFile($class);
        }
        if ($return) {
            return $return;
        }
    }

    MissingClassLookup::trackMissingClass($class);
}

As for an example of code that is not compatible with multiple PHP versions:

  • The very nature of the version numbering system is that when "breaking" changes are introduced, a major number revision is required. While at-times these changes are simply deprecated classes (which can be replaced with if...else statements), many end up being parse errors.
  • My original commit had a Parse Error with PHP 5.3, but was fully compatible with 7.x (see Build #6897)
  • Many things listed in PHP 7.0: New features will cause a Parse Error in PHP 5.x

@emeraldinspirations
Copy link
Contributor Author

This was just a suggestion anyway. Only way to learn is to try! 😄

@Seldaek
Copy link
Member

Seldaek commented Jun 17, 2017

About chaining autoloaders, the problem is they are not necessarily chaining Composer autoloaders, but people might have their own autoloaders registered via spl_autoload_register as well, for handling some other classes. I don't see how refactoring and adding a bunch of interfaces and whatnot would solve any of that to be honest.

And indeed I realize that new syntax in newer versions is not supported in old ones, but generally speaking this new syntax rarely enables new functionality, it just makes things easier to achieve with less code, but if you are going to code it for a lower version anyway, then I'd argue it's best to just use that for all php versions than to have two copies of the code to maintain..

Anyway no hard feelings, it's ok to make suggestions :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants