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

What should be the minimum PHP version for Cypht 3.0? PHP 8.1? 8.2? 8.3? #977

Closed
marclaporte opened this issue Apr 26, 2024 · 22 comments
Closed
Assignees
Milestone

Comments

@marclaporte
Copy link
Member

Cypht 1.3.x requires PHP 5.4
https://github.com/cypht-org/cypht/blob/release-1.3.0/composer.json

Cypht 1.4.x requires PHP 5.6
https://github.com/cypht-org/cypht/blob/1.4.x/composer.json#L31

As of now, Cypht master requires PHP 7.4
https://github.com/cypht-org/cypht/blob/master/composer.json#L39

Cypht master will branch to Cypht 2.0 soon.

So then master will be the future Cypht 3.0

Related: https://doc.tiki.org/PHP-8

@marclaporte
Copy link
Member Author

@jonocodes
Copy link
Contributor

What is the state of php 8.x?
Has cypht been run in 8 to see if it works or needs updating?

@marclaporte marclaporte added this to the 3.0 milestone May 5, 2024
@marclaporte
Copy link
Member Author

I just officialized that Cypht 1.4.x is now an LTS version so end users don't have to rush to upgrade:
https://github.com/cypht-org/cypht/wiki/Lifecycle

@marclaporte
Copy link
Member Author

What is the state of php 8.x?
Has cypht been run in 8 to see if it works or needs updating?

Most (maybe all) active devs use PHP 8.x so they will fix any bugs they encounter. And we have seen such commits. A risk is that they don't also make sure to maintain support for PHP 7.4, which is one more reason to ditch 7.4. Since we now have Cypht LTS, users will have security patches for a while if they prefer waiting before proceeding to a major upgrade.

@marclaporte
Copy link
Member Author

So then master will be the future Cypht 3.0

That was true when I wrote it, but since then, we have improved https://github.com/cypht-org/cypht/wiki/Lifecycle so we'll only do a major revision if the code changes justify it.

So now, a likely scenario is that master becomes 2.1.0 and then 2.2.0 and then 2.3.0. At this point, we'll probably be itching for bigger changes, so then master will become 3.0.

@jonocodes
Copy link
Contributor

I asked because I am specifically looking at some error handling, that I would presume breaks things:
https://php.watch/versions/8.0/PDO-default-errmode

Should I bring this up in another discussion or is there a place for general php 8 discussions?

@marclaporte
Copy link
Member Author

Here is good.

@jonocodes
Copy link
Contributor

jonocodes commented May 7, 2024

ok. Concerning this PDO error modes php.watch/versions/8.0/PDO-default-errmode.

It appears php 7.x defaulted to make them silent, but 8.x is making them throw exceptions. This would have very different behaviors in the way cypht uses them since it is not explicitly setting a mode. So I recommend setting it explicitly.

That being said, I agree with the decision to have it default to throw exceptions. I came upon this when trying to figure out why some database operations were not working while I was developing. cypht seems to capture and hide the actual errors as you can see here:

if ($auth->create($user, $pass) === 2) {
die("User '" . $user . "' created\n\n");
}
else {
print_r(Hm_Debug::get());
print_r(Hm_Msgs::get());
die("An error occured when creating user '" . $user . "'\n\n");
}

Unless there is another way to expose these errors (perhaps via a debug mode log) you cant see what the issue is, which is needed in order to fix it.

In my testing I was able to turn the above into a single line:

	$auth->create($user, $pass);

I explicitly ask for exceptions to happen when setting up the connection:

self::$dbh[$key]->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

That way the (error) logs are actionable.

@jonocodes
Copy link
Contributor

jonocodes commented May 7, 2024

A seperate issue I am running into that may be php 8 related, though I am not sure. The INSTALL file says to run
php scripts/config_gen.php

when I do this I get:

[jono@dobro:~/src/cypht]$ php scripts/config_gen.php 

WARNING: No PHP Redis support found, Redis caching or sessions will not work


WARNING: No PHP Memcached support found, Memcached caching or sessions will not work


WARNING: No PHP gnupg support found, The PGP module set will not work if enabled

scanning module "core ...
scanning module contacts ...

Fatal error: Uncaught Error: Call to undefined function setup_base_page() in /home/jono/src/cypht/modules/contacts/setup.php:10
Stack trace:
#0 /home/jono/src/cypht/scripts/config_gen.php(193): require()
#1 /home/jono/src/cypht/scripts/config_gen.php(99): get_module_assignments(Array)
#2 /home/jono/src/cypht/scripts/config_gen.php(31): build_config()
#3 {main}
  thrown in /home/jono/src/cypht/modules/contacts/setup.php on line 10

[jono@dobro:~/src/cypht]$ php -v
PHP 8.2.17 (cli) (built: Mar 12 2024 14:26:30) (NTS)

My concern is the error. The warnings are also an issue, but I dont want to overload this conversation with that part.

@marclaporte
Copy link
Member Author

@Shadow243 Please advise

@Shadow243
Copy link
Member

Fatal error: Uncaught Error: Call to undefined function setup_base_page() in /home/jono/src/cypht/modules/contacts/setup.php:10
Stack trace:
#0 /home/jono/src/cypht/scripts/config_gen.php(193): require()
#1 /home/jono/src/cypht/scripts/config_gen.php(99): get_module_assignments(Array)
#2 /home/jono/src/cypht/scripts/config_gen.php(31): build_config()
#3 {main}
thrown in /home/jono/src/cypht/modules/contacts/setup.php on line 10

[jono@dobro:~/src/cypht]$ php -v
PHP 8.2.17 (cli) (built: Mar 12 2024 14:26:30) (NTS)


My concern is the error. The warnings are also an issue, but I dont want to overload this conversation with that part.

@jonocodes, did you add some configurations? I am not able to reproduce this issue. The only scenario where this is possible is when we comment out:

require_once APP_PATH.'modules/core/functions.php';

from modules/core/setup.php, which we are scanning successfully the first time. If not, then we should also import require_once APP_PATH.'modules/core/functions.php'; in scripts/config_gen.php; Can we have your specifications (environment in which you are encountering this) ?

Nevertheless, I have some issues with php 7.4 and 8.0.

sudo php80 scripts/config_gen.php                                                                                                                            ─╯

Warning: preg_match(): Allocation of JIT memory failed, PCRE JIT will be disabled. This is likely caused by security restrictions. Either grant PHP permission to allocate executable memory, or set pcre.jit=0 in /Users/shadow243/software/www/TIKI/cypht/vendor/symfony/dotenv/Dotenv.php on line 425

There is no error from 8.1

@jonocodes
Copy link
Contributor

@Shadow243

require_once APP_PATH.'modules/core/functions.php';

from modules/core/setup.php, which we are scanning successfully the first time. If not, then we should also import require_once APP_PATH.'modules/core/functions.php'; in scripts/config_gen.php

Adding require_once APP_PATH.'modules/core/functions.php'; to scripts/config_gen.php fixed it for me. Thanks.

When you say "we are scanning successfully the first time", what does that mean? What is doing the scanning?

@kroky
Copy link
Member

kroky commented May 10, 2024

If we support PHP7.4 and PHP8 without proper error catching of PDO exceptions, I suggest we stay with explicit SILENT behavior as it used to be AND create a new PR for master/2.0.x (could be backported to 1.4 as well) where we turn on the exception error mode and add code to handle it correctly.

@jonocodes
Copy link
Contributor

jonocodes commented May 10, 2024

@kroky
Sounds good. I moved the error handling discussion to a new ticket #1017

@Shadow243
Copy link
Member

as planed here: [https://avan.tech/item106800](PHPunit upgrade), we will either set 8.1 for phpunit 10 nor 8.2 for phpunit 11

@marclaporte
Copy link
Member Author

Interesting: https://phpunit.de/supported-versions.html

As of now, none of the Tiki versions require PHP 8.2+ (but some support it)
https://doc.tiki.org/Requirements#PHP_

Cypht running in Tiki is more important than PHPUnit 11. We could bump Tiki requirements to PHP 8.2 for Tiki 28 or Tiki29, but that is a community discussion. So as of now, I think the best is for Cypht to align with PHP requirements of Tiki. Tiki 27.x LTS requires PHP 8.1 so let's go to PHPUnit 10.

@marclaporte
Copy link
Member Author

Thank you @Shadow243 for #1044

@marclaporte
Copy link
Member Author

I see #1044 already bumps requirement to PHP 8.1, closing this as done.

@marclaporte
Copy link
Member Author

I updated https://github.com/cypht-org/cypht/wiki/Lifecycle accordingly.

@TheDigitalOrchard
Copy link
Contributor

PHP 8.x has already been stable for 3 years, and I've noticed many libraries now defaulting to a minimum version of PHP 8.x. PHP 7.x has already passed its EOL.

While it would be a "nice" thing to continue to support PHP 7.x with Cypht, I think the best option is to keep Version 2.x around for that purpose and Version 3.x sets a minimum version of PHP 8.1.

Just my take.

@Shadow243
Copy link
Member

PHP 8.x has already been stable for 3 years, and I've noticed many libraries now defaulting to a minimum version of PHP 8.x. PHP 7.x has already passed its EOL.

While it would be a "nice" thing to continue to support PHP 7.x with Cypht, I think the best option is to keep Version 2.x around for that purpose and Version 3.x sets a minimum version of PHP 8.1.

Just my take.

We are not able to upgrade phpunit to 10 with php 7.4 bcz the minimum php version for phpunit 10 is 8.1

@marclaporte
Copy link
Member Author

While it would be a "nice" thing to continue to support PHP 7.x with Cypht, I think the best option is to keep Version 2.x around for that purpose and Version 3.x sets a minimum version of PHP 8.1.

Yes, and that is what we are doing. Plus Cypht 1.4.x with support for PHP 5.6+. I added the supported PHP versions here:
https://github.com/cypht-org/cypht/wiki/Lifecycle

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

No branches or pull requests

5 participants