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

Autoload different files for a class for different PHP-Versions #4385

Closed
heiglandreas opened this issue Aug 31, 2015 · 11 comments
Closed

Autoload different files for a class for different PHP-Versions #4385

heiglandreas opened this issue Aug 31, 2015 · 11 comments
Labels

Comments

@heiglandreas
Copy link

Would it be possible to enable composer to handle PHP-Versions on autoloading?

Background: I'm developing a library and want to handle f.i. DateTime-related stuff. For PHP5.4 I'd check for DateTime using instanceof but for PHP5.5 I'd have to consider DateTimeImmutable as well and might want to refer to DateTimeInterface instead. Or I want to start implementing new PHP7-Stuff and keep it separate from the PHP5.x-stuff.

Therefore it would be great to have the possibility to create the class twice, once for PHP5.4 and once for PHP>=5.5 and have composer decide which class to serve depending on the currently running PHP-Version.

I couldn't find an issue handling such a feature, therefore I created it. If it already existed and has been disapproved, I'm sorry for the disturbance.

@stof
Copy link
Contributor

stof commented Aug 31, 2015

Why couldn't you make a single class definition supporting both ? This would avoid lots of duplication in the project (you would have to ensure that all your implementations stay in sync).

Regarding the handling of DateTime, this is easy:

if ($arg instance of \DateTime || $arg instance \DateTimeImmutable) {
    // do something with the date
}

This code works on PHP 5.3+

@staabm
Copy link
Contributor

staabm commented Aug 31, 2015

This could also be a good case for class_alias depending on the PHP-version while bootstrapping.

@heiglandreas
Copy link
Author

@stof DateTime perhaps wasn't the best example. Take f.i. scalar type hinting and you are lost with doing it in a single file. It will break in PHP<7.

@stof
Copy link
Contributor

stof commented Aug 31, 2015

@heiglandreas if you have a typehinted implementation and a non-typehinted one in the same version of your package, this is impossible for semver: your class has a different signature based on the version, which breaks any code extending it if they don't also do the same weird thing than you. Changing the signature of a class is a BC break, and should be done in a new major version of the package to respect semver. Doing it based on the PHP version in a single package version will lead to many WTF and make it impossible for other packages to write a proper requirement for your package.

@heiglandreas
Copy link
Author

It doesn't need to have a different signature, as I can "define" expected types for a function/method using DocBlock. So technically the signature doesn't change at all. It's just the way I'm asserting that the correct types are given that changes. While I had to do lots of type-juggling at the start of my method to assure I've got the right types I can now ommit all that stuff. But that needs a certain PHP-Version.

Regard this code which runs on most PHP-versions

/**
 * @param int $a
 * @param int $b
 * @return int
 */
public function add($a, $b) {
    if (! is_int($a) {
        trigger_error('Argument 1 passed to add must be an integer', E_RECOVERABLE_ERROR);
    }
    if (! is_int($b) {
        trigger_error('Argument 2 passed to add must be an integer', E_RECOVERABLE_ERROR);
    }

    return $a + $b;
}

and this code which required PHP7

public function add(int $a, int $b): int {
    return $a + $b;
}

The method signature is de-facto the same so there would be no need for semver to use a new major version.

I could even do such ugly thing as this:

if (version_compare(PHP_VERSION, '7.0.0', < )) {
    /**
     * @param int $a
     * @param int $b
     * @return int
     */ 
    public function add($a, $b) {
        if (! is_int($a) {
            trigger_error('Argument 1 passed to add must be an integer', E_RECOVERABLE_ERROR);
        }
        if (! is_int($b) {
            trigger_error('Argument 2 passed to add must be an integer', E_RECOVERABLE_ERROR);
        }

        return $a + $b;
    }
} else {
    public function add(int $a, int $b): int {
        return $a + $b;
    }
}

to do the same but it's rather ugly....

Being able to provide different Versions for autoloading depending on PHP-Version would be much easier and smarter.

@stof
Copy link
Contributor

stof commented Aug 31, 2015

@heiglandreas try to extend you class and overwrite the method (with error_reporting set to E_ALL), and you will see that the PHP signature is different (the fact that you document a type in the docblock does not make it part of the signature know by the PHP runtime).

btw, you cannot put the if that way. It has to be around the whole class (and if these functions are not methods on a class, the public keyword is invalid and the discussion is irrelevant as the autoloading only deals with classes)

@heiglandreas
Copy link
Author

@stof I ommited the class-declaration for brevity reasons.

But as @staabm already pointed out it might be better to use class-aliases for that purpose.

So you could use (not tested at all)

namespace Foo\PHP7;
class Foo {}

namespace Foo\PHP5;
class Foo {}

namespace Foo;
if (version_compare(PHP_VERSION, '7.0.0alpha1', '<') {
    class_alias('\Foo\PHP5\Foo', '\Foo\Foo');
} else {
    class_alias('\Foo\PHP7\Foo', '\Foo\Foo');
}

That way you can use different classes for different versions but they are properly namespaced and can therefore be extended as wished. And that wouldn't require any magic on composers side.

On extending the classes you will have to do the same though otherwise you'd only extend one of the two classes…

So for using the lib you can refer to \Foo\Foo and depending on the PHP-Version you'll get the "optimized" one but for extending the lib you can use the appropriate class you'd like to overwrite depending on the PHP-version.

Working example can be found at https://3v4l.org/1PNlp

@ThYpHo0n
Copy link

ThYpHo0n commented Sep 5, 2015

But class aliases doesn't solve to deliver just one lib for each PHP version. Definitely this is a minor thing I would admit but Composer knows the given PHP version and could load only the libs that fit to that PHP version and in case of an lower PHP version there is no way I know right now to put a fallback lib in place.

A small example:
Lib A requires PHP 7 and implements some FooBar
Lib B requires PHP 5 and implements some FooBar
Project A requires FooBar and if the current PHP version is >= 7 then use Lib A otherwise use Lib B.

Right now you need to add both Libs into your project and use aliases for it and do the PHP version check on your own.

@Seldaek
Copy link
Member

Seldaek commented Feb 20, 2016

Closing as very unlikely we will implement this.

@Seldaek Seldaek closed this as completed Feb 20, 2016
@emeraldinspirations
Copy link
Contributor

I am new to GitHub, so please excuse me if this question is inappropriate.

I submitted a pull request #6455 containing a suggested fix for this issue. Said pull request was accepted. Does that then somehow change this from "closed" to "resolved" or something?

@alcohol
Copy link
Member

alcohol commented May 29, 2017

@emeraldinspirations it was not yet accepted. It is still pending. The changes look reasonable, but they could still be rejected at this point.

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

No branches or pull requests

7 participants