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

Allow subclasses to set their own $_config property #77

Closed
wants to merge 1 commit into from
Closed

Allow subclasses to set their own $_config property #77

wants to merge 1 commit into from

Conversation

markchalloner
Copy link

Allow subclasses to set their own $_config property via configure (requires PHP 5.3), required by j4mie/paris#33.

Bumped PHP requirement in readme.

…quires PHP 5.3), required by j4mie/paris#33. Bumped PHP requirement in readme.
@ghost ghost assigned treffynnon Nov 26, 2012
@treffynnon
Copy link
Collaborator

This is the route I was thinking of. Like you say it does mean it will no longer be compatible with PHP 5.2. There are a number a couple of features that I would like to release before bumping up the PHP version requirement so people still on 5.2 can get them.

@markchalloner
Copy link
Author

Could fake it if you need to keep compatibility. It's hacky though and you would need to have something in subclasses like Paris when < 5.3:

ORM:

public static function configure($key, $value=null) {
    // LSB supported
    if (function_exists('get_called_class')) {
        self::_configure(null, $key, $value);
    // Backward compatibility for PHP < 5.3 - requires overriding configure in subclasses
    } else {
        self::_configure(__CLASS__, $key, $value);
    }
}

// Renamed
public static function _configure($class, $key, $value=null) {
    // Shortcut: If only one argument is passed,
    // assume it's a connection string
    if (is_null($value)) {
        $value = $key;
        $key = 'connection_string';
    }
    if ($class) {
        $class::$_config[$key] = $value;
    } else {
        static::$_config[$key] = $value;
    }
}

ORMWrapper (if < 5.3):

public static function configure($key, $value=null) {
    self::_configure(__CLASS__, $key, $value);
}

@treffynnon
Copy link
Collaborator

Thank you for this pull request, but...

I am getting a lot of calls not to break the backwards compatibility of Idiorm and Paris and moving to PHP 5.3 would do this.

A number of users have contacted me to say that they regularly use Idiorm and Paris in proof of concepts or in small projects, which end up on shared hosting. With this in mind I am loathe to bump the PHP requirement despite 5.2 having hit EOL over two years ago. There are still a large number of hosts out there running PHP 5.2.

If we were to bump the requirement then I feel there would need to be a big justification - more than overriding the configuration, which is possibly nullified by the pending multiple connections pull request (#76) anyway. @tag is still working on it, but once complete I think it will cover this use case completely and still provide support for PHP 5.2.

When I first began maintaining Idiorm I underestimated the importance that many users had placed on its ability to support PHP 5.2. There again no one likes backwards compatibility breaks that mean they cannot access the latest features so it shouldn't really have come as too much of a surprise - especially given that I still run a 5.2 server myself!

With all of this in mind it is unlikely that this pull request will be merged into the core repository any time soon or even that a release moving away from PHP 5.2 support will ever be forthcoming. Unless there is a big pressing need as I mentioned before.

Idiorm has now got to a point where I would say that it is overstepping its original philosophy already (instead of 20/80 we are more like 10/90).

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.

2 participants