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

Add PHP version requirement to composer.json #139

Conversation

danielTiringer
Copy link

@danielTiringer danielTiringer commented May 16, 2024

Summary of Changes

The recently released version 6 contains changes that are only available since PHP 8.1, like initializing class variables in the constructor (see this RFC ). The code example below is from LtiMessageLaunch:

    public function __construct(
        private IDatabase $db,
        private ICache $cache,
        private ICookie $cookie,
        private ILtiServiceConnector $serviceConnector
    ) {
        $this->launch_id = uniqid('lti1p3_launch_', true);
    }

Having a php version designated in the composer.json throws an error if someone with an out-of-date php version attempts to require the package, providing an early warning that they wouldn't be able to use it due to the newer language constructs implemented.

Testing

  • I have added automated tests for my changes - N/A
  • I ran composer test before opening this PR - N/A
  • I ran composer lint-fix before opening this PR - N/A

@dbhynds dbhynds self-assigned this May 21, 2024
@dbhynds dbhynds self-requested a review May 21, 2024 19:39
Copy link
Member

@dbhynds dbhynds left a comment

Choose a reason for hiding this comment

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

You'll want to update https://github.com/danielTiringer/lti-1-3-php-library/blob/add-php-version-requirement-to-composer-json/.github/workflows/run_tests.yml#L27 to 8.1, since 8.0 will now always fail.

Otherwise, this looks good! Thanks for contributing

@dbhynds
Copy link
Member

dbhynds commented May 22, 2024

I fixed the broken test already on master. Merging this in.

Thanks for contributing!

@dbhynds dbhynds merged commit 06f7723 into packbackbooks:master May 22, 2024
3 of 5 checks passed
@danielTiringer danielTiringer deleted the add-php-version-requirement-to-composer-json branch May 22, 2024 16:41
@danielTiringer
Copy link
Author

Thank you too!

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