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 PHP 8.4 in laminas-cache 3.x #357

Merged
merged 3 commits into from
Jan 21, 2025
Merged

Conversation

driehle
Copy link

@driehle driehle commented Jan 10, 2025

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA yes

Description

This MR adds support for PHP 8.4 to laminas-cache v3. Adding support to the legacy version is essential, as laminas-cache v4 requires servicemanager v4 and many users cannot yet migrate to servicemanager v4, as quite a few packages in the MVC ecosystem yet lack support for servicemanager v4. Hence, it makes much sense to add support for PHP 8.4 to laminas-cache v3, given that this is pretty straight foward.

Signed-off-by: Dennis Riehle <[email protected]>
@driehle driehle force-pushed the feat/php84 branch 2 times, most recently from fc7ad18 to 593e04b Compare January 10, 2025 16:49
@driehle
Copy link
Author

driehle commented Jan 10, 2025

@froschdesign

This is a follow-up on doctrine/DoctrineORMModule#767, on how PHP 8.4 support could be added to laminas-cache 3.x series. Technically it works, there are, however, some issues:

Due to the architecture of laminas-cache, there is a dependency on the root project. Therefore, Composer needs to inflect the version of the root package and, obviously, fails to do so. The initial error in CI was:

    - laminas/laminas-cache-storage-adapter-filesystem 2.4.1 requires laminas/laminas-cache ^3.10.0 
      -> found laminas/laminas-cache[4.2.x-dev] but it does not match the constraint. 
           See https://getcomposer.org/dep-on-root for details and assistance.

So it seems to inflect 4.2.x-dev, though 3.13.x-dev is what the value should be. First of all, there was typo in the pre-install.sh script, where COMPOSER_ROOT_VERISON was set. However, I think this script does not have any effect at all, at least fixing the typo doesn't change anything. I think this makes sense, since variables exported in Bash are lost when the script exits. So when the actuall Composer call is made, the variable will not be present anymore.

For now, I have set the version in composer.json, which is a bit hacky and ugly. A clean solution would probably be to integrate the code from the pre-install.sh script somwhere in the laminas-ci-matrix-action repository, so that COMPOSER_ROOT_VERSION can actually be set based on the target branch of the MR when Composer is called.

With the version pinned, most CI checks work. The docs seem to be failed, but I'd call this unrelated. The Psalm baseline probably needs to be updated, which I haven't done yet and the backward compatibility check, I have no idea what this is about. 🤷

@driehle driehle changed the title allow PHP 8.4 Allow PHP 8.4 in laminas-cache 3.x Jan 10, 2025
@driehle driehle force-pushed the feat/php84 branch 3 times, most recently from 883eff2 to 333a898 Compare January 13, 2025 18:48
@driehle
Copy link
Author

driehle commented Jan 13, 2025

I tried setting COMPOSER_ROOT_VERSION as @boesing suggested in laminas/laminas-continuous-integration-action#35, i.e. the GitHub way by writing to $GITHUB_ENV. However, this doesn't work and after thinking about it, in indeed does make sense that it does not work: Both composer install and the following phpunit call are executed in the same step, as there is only one CI step, which is the laminas/laminas-continuous-integration-action handling all the stuff in one single step. And according to GitHub docs the GITHUB_ENV is only for passing variables between different steps.

Therefore, I am surprised how the code in https://github.com/laminas/laminas-cache-storage-adapter-blackhole/blob/fe7bb4045fae6a71f8179744829aab6ddf47521e/.laminas-ci/composer-root-version.sh was supposed to work. In the same way, I am surprised how the code in this repository, using export COMPOSER_ROOT_VERSION, should ever have worked. Variables set in bash scripts are lost, once the script exits. This would only be possible, if the script is sourced, but that is not happening in https://github.com/laminas/laminas-continuous-integration-action/blob/1.41.x/entrypoint.sh#L176.

@driehle
Copy link
Author

driehle commented Jan 20, 2025

@gsteel @samsonasik Any suggestions on how to proceed here?

@gsteel
Copy link
Member

gsteel commented Jan 20, 2025

Hi @driehle - In laminas-view we checkout a new branch prior to running CI: https://github.com/laminas/laminas-view/blob/2.37.x/.laminas-ci/pre-install.sh

@driehle
Copy link
Author

driehle commented Jan 20, 2025

Great idea, that works like a charm! 👏

Now all tests except for the docs linting are passing. I won't fix the docs linting though, as Laminas only renders docs from the latest branch (i.e. currently 4.2.x) and docs from legacy branches, like 3.13.x here, are not used anyways. Do you agree?

@driehle driehle marked this pull request as ready for review January 20, 2025 22:31
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thanks @driehle - If we can keep the diff just to dot files and tooling stuff, the merge-up will be cleaner 🤞

Also, yes to leaving the docs linting errors alone 👍

src/Storage/Capabilities.php Outdated Show resolved Hide resolved
test/Storage/Adapter/AbstractAdapterTest.php Outdated Show resolved Hide resolved
@driehle driehle requested a review from gsteel January 21, 2025 12:17
@gsteel gsteel added this to the 3.13.0 milestone Jan 21, 2025
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thank you @driehle 👍

@driehle
Copy link
Author

driehle commented Jan 24, 2025

I missed that we need laminas-serializer as well to get laminas-cache running with PHP 8.4. So there are two more PRs on laminas-serializer 2.x now, of which the first we'll need to merge as well. The second is rather just a suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants