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

Deprecate php-4-style constructors #11

Merged
merged 2 commits into from
Nov 11, 2015
Merged

Deprecate php-4-style constructors #11

merged 2 commits into from
Nov 11, 2015

Conversation

derrabus
Copy link
Contributor

@derrabus derrabus commented Nov 2, 2015

In php 4 the name of the constructor function had to match the class name. php 5 introduced __construct() as new constructor function for classes that should be used instead.

Now, php 7 deprecates the php-4-syle constructors. That means, a E_DEPRECATED warning is triggered if such a function is still in use. Furthermore, a future php release (probably 7.1) will remove support for those constructor functions.

This PR adds __construct() to all classes. Since simply renaming all constructor functions will break code with classes that extend one of the Log_* classes, referring to the old constructor, the old constructor is kept in place, simply calling the new one.

I have also added a __construct() method to the Log class. This might seem a bit odd at first. I did that because php 7 interprets the presence of the log() method as old-style constructor (d'oh), so adding a __construct() method here avoids an E_DEPRECATED warning.

@CloCkWeRX
Copy link
Member

I think it'd be OK to just drop the old style constructors entirely - we can just raise the min supported php version.

@derrabus
Copy link
Contributor Author

derrabus commented Nov 4, 2015

The minimal php version right now is 5.2 (according to composer.json) or 5.0 (according to package.xml), which is just fine for the new constructors.

My intention was not to support lower versions of php. I simply did not want to introduce a breaking change. For instance, if someone would have written code like this:

class Log_my_special_file extends Log_file
{
    function Log_my_special_file($name, $ident = '', $conf = array(), $level = PEAR_LOG_DEBUG)
    {
        parent::Log_file($name, $ident, $conf, $level);
    }

    // Here be more dragons
}

This might not be the intended way to extend this library, but it's possible. If we now rename all constructors, that piece of code will break. This is why I left the old constructor functions in place, as a kind of compatibility layer. They don't hurt, yet you would give people with code like the piece above some time to migrate to the new constructors before removing the old ones in a later release.

@jparise
Copy link
Member

jparise commented Nov 9, 2015

I thought about this a bit, and I'm comfortable starting the 1.13.x release line with this change without the name-based compatibility layer. We can also use the opportunity to correct the inconsistent PHP version requirements.

@derrabus
Copy link
Contributor Author

All right then, I'm going to remove the compatibility layer.

@derrabus
Copy link
Contributor Author

The compatibility layer is gone now.

CloCkWeRX added a commit that referenced this pull request Nov 11, 2015
@CloCkWeRX CloCkWeRX merged commit b1ec3a7 into pear:master Nov 11, 2015
@CloCkWeRX
Copy link
Member

Looking at the package.xml; php5+ is already required

@derrabus derrabus deleted the deprecate-legacy-constructors branch November 25, 2015 09:59
@derrabus
Copy link
Contributor Author

Any plans for a stable release with this patch?

@jparise
Copy link
Member

jparise commented Apr 3, 2016

Sorry for the delay @derrabus. I just published version 1.13.0.

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.

3 participants