[RFC] Refactoring phpbb_version_compare

[3Di]

Hi all,
the purpose is to refactor the function phpbb_version_compare in order to check if at least also the files (constants.php) are accordingly to the DB's config['version'].

Gives a more constructive way to check, at least, if we are allowing to install something onto a not ready to be used Board that gives the supporters more troubles than nothing else, nonetheless.

Making it mandatory the ext.php for extensions isn't, IMO, a bad idea.. also.

Comments and suggestions are welcome.
Thanks.

[david63]

I would agree with using the ext file for this - it can be customised to give more meaningful, and helpful, error messages to the user.

[3Di]

At the present time and since has been introduced (phpBB 3.0.10-RC1) this function is just a wrapper to the PHP version_compare. Only difference is that is case-insensitive, AFAIK.

The ext.php file also uses the phpbb_version_compare, that's why the present RFC.
This leads also me to think if it could be better to refactor the function is_enableable, instead.

Code: Select all

	public function is_enableable()
	{
		$config = $this->container->get('config');
		return phpbb_version_compare($config['version'], '3.1.8-RC1', '>=');
	}

[Marc]

A change for comparing these two versions has been proposed for 3.1.x already:
https://github.com/phpbb/phpbb/pull/3901

The PR is currently waiting small fixes to it's initial proposal though.

[MattF]

phpbb_version_compare should not be changed. It doesn't care what you're comparing. It's just a case-insensitive wrapper for version_compare.

And refactoring is_enableable is also not an likely, as it is meant for any possible reason an extension can be allowed to enable or not. It is purposely ambiguous by default and up to ext authors to decide how they want to use it, if they need it.

Also, the eventual goal is for extensions to respect the composer.json version constraints when been installed. Using is_enableable() to compare the phpbb version is just a stop-gap measure I came up with until such time.

I would agree with using the ext file for this - it can be customised to give more meaningful, and helpful, error messages to the user.

It already does. It basically says something about the board not meeting the requirements of the extension.

[david63]

It basically says something about the board not meeting the requirements of the extension

The problem with that message is is that it is too generic - by using the ext file you can have multiple tests each with their own error message.

What would make life easier would be if an extension developer could add an optional language variable to the phpbb_version_compare function to override the generic message.

[MattF]

Umm, look at phpbb_version_compare. It only returns a boolean true/false. It has nothing to do with the messages displayed.

Also, no extension can add any language variables if the extension is not enabled.

But like I said before, the is_enableable() doing a version check is temporary until 3.2/3.3. And of course it's message is a little generic, there's a lot of reasons why one might return false for is_enableable. But in fact the message is right, if is_enableable is false, it means the extension can not be allowed to be enabled on the board. Doesn't matter what tests resulted in false really, the bottom line is that extension fails to meet a requirement for being enableable. And since the extension is not enabled, it can't provide it's own message, so it relies on phpBB's message.

And just to say it a third time, this is all moot, as this will all be a thing of the past soon enough. Thanks to https://github.com/phpbb/phpbb/pull/3909

[3Di]

A change for comparing these two versions has been proposed for 3.1.x already:
https://github.com/phpbb/phpbb/pull/3901

The PR is currently waiting small fixes to it's initial proposal though.

Thanks Marc for the link, I am seeing also here the good job done and the fact that's related to Ascraeus, though (for completeness) it could include a check also in cases (like I encountered) that for some reasons the user ran the DB updater but the files weren't updated, believe it or not.

Also this, though, isn't related to what I meant opening this Topic.

I am trying to re-word/explain the whole thing also having read further this topic, till the end.

I will post an exaustive answer to those points once some test has been completed in my loc env, thank you.

[3Di]

I am still WIP about the new function to use instead of phpbb_version_compare, that still will remain in place into the core code. Just a one more and accurated function that serves in this cases, after having read all of your comments.

What it is also worth to be mentioned is that a lot of requests for support are related to installed Ascraeus Boards onto a server running PHP7, that shouldn't happen IMO.

There could be also a check into the installer that denies the installation at all in this case.

Just a thought.

Reference (just one of a lot) : https://www.phpbb.com/community/viewtop ... #p14404376

Thanks.

[3Di]

What it is also worth to be mentioned is that a lot of requests for support are related to installed Ascraeus Boards onto a server running PHP7, that shouldn't happen IMO.

There could be also a check into the installer that denies the installation at all in this case.

https://tracker.phpbb.com/browse/PHPBB3-14596