Skip to content

[Requirement] Move validate php version check to CodeIgniter class#4426

Merged
MGatner merged 2 commits intocodeigniter4:developfrom
samsonasik:add-php-requirement-class
Mar 15, 2021
Merged

[Requirement] Move validate php version check to CodeIgniter class#4426
MGatner merged 2 commits intocodeigniter4:developfrom
samsonasik:add-php-requirement-class

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented Mar 13, 2021

By centralize check, no need to change in separate files between spark and public/index.php anymore.

Checklist:

  • Securely signed commits

@samsonasik samsonasik changed the title [Requirement] MCreate CodeIgniter\PhpRequirement::validatePHPVersion() static method to check minimum valid PHP version [Requirement] Create CodeIgniter\PhpRequirement::validatePHPVersion() static method to check minimum valid PHP version Mar 13, 2021
Copy link
Copy Markdown
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This moves the check later in both entry points. Is there anything that would fail on lower PHP versions in between?
  2. What does a dedicated class have to offer? We already track the framework version on CodeIgniter\CodeIgniter, seems like it might be a logical place to group this: const CI_VERSION = '4.1.1';

@samsonasik
Copy link
Copy Markdown
Member Author

  1. lower php version will be fail, eg in php 5.2, probably just use function and check very early (need require_once)
  2. yes, that can be in CodeIgniter class, what do you prefer?

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Mar 14, 2021

I guess I'm not too worries about PHP < 7, as long as executed code will work up until the check in 7+. This will become more relevant as we drop versions moving forward, so for example if someone does not read the release notes and makes a point upgrade that breaks their app at least they will know why.

I would be in favor of consolidating this to CodeIgniter because of our framework version tracking there, but it's not a strong opinion. We can see what others think.

@samsonasik
Copy link
Copy Markdown
Member Author

@MGatner I've moved the check to CodeIgniter class 077ca27

@samsonasik samsonasik changed the title [Requirement] Create CodeIgniter\PhpRequirement::validatePHPVersion() static method to check minimum valid PHP version [Requirement] Move validate php version check to CodeIgniter class Mar 14, 2021
@samsonasik
Copy link
Copy Markdown
Member Author

All green 🎉

@samsonasik samsonasik requested a review from MGatner March 14, 2021 20:01
Copy link
Copy Markdown
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version constraint looks good, thank you for moving it! I'm not sure what has happened with the explosion of use statement changes but I would prefer to see all that in a separate PR.

@mostafakhudair
Copy link
Copy Markdown
Contributor

@MGatner CLI is accessible without php version check

Comment thread system/CodeIgniter.php
{
die(
sprintf(
'Your PHP version must be %s or higher to run CodeIgniter. Current version: %s',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible, if this is added to Lang?

Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Mar 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totoprayogo1916 if you mean for multi language, I am not sure since it early executed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lang() is definitely not available yet. In general I'm okay with strictly internal messages being in English. I understand there are some internationalization efforts for PHP core but I think this is still pretty common?

@samsonasik samsonasik force-pushed the add-php-requirement-class branch from abae0fd to 077ca27 Compare March 15, 2021 02:10
@samsonasik
Copy link
Copy Markdown
Member Author

@MGatner I create separate PR for rector notice #4430

@samsonasik samsonasik requested a review from MGatner March 15, 2021 02:15
@samsonasik samsonasik force-pushed the add-php-requirement-class branch from 077ca27 to bd65ddf Compare March 15, 2021 06:12
@samsonasik
Copy link
Copy Markdown
Member Author

rebased.

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Mar 15, 2021

Looks much better! Thanks for splitting that. What about @mostafakhudair's concern that CLI execution bypasses version check?

@samsonasik
Copy link
Copy Markdown
Member Author

@mostafakhudair any step to reproduce for by pass via cli ? I can't reproduce via spark command.

@mostafakhudair
Copy link
Copy Markdown
Contributor

@samsonasik @MGatner everything is working well now, I didn't know what was the problem.

@MGatner MGatner merged commit c610fe0 into codeigniter4:develop Mar 15, 2021
@samsonasik samsonasik deleted the add-php-requirement-class branch March 16, 2021 00:53
totoprayogo1916 added a commit to totoprayogo1916/framework-CI4 that referenced this pull request May 11, 2021
Since codeigniter4#4426, this docs shoul updated.
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.

5 participants