Skip to content

compat fixes for CakePHP 5.3.0+ and PHP 8.4+#36

Open
lordphnx wants to merge 1 commit intojeremyharris:masterfrom
lordphnx:master
Open

compat fixes for CakePHP 5.3.0+ and PHP 8.4+#36
lordphnx wants to merge 1 commit intojeremyharris:masterfrom
lordphnx:master

Conversation

@lordphnx
Copy link

Hey Jeremy, hope you are well.

Encountered a small compatibility issue due to CakePHP 5.3 apparently adding an override for __get in EntityTrait.
Running the existing test suite after a composer update failed tests.

This is the smallest update I could think of. If you disagree, would love to hear.

As always your time is much appreciated!

Copy link
Owner

@jeremyharris jeremyharris left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I took a look and don't quite understand the changes. Can you possible post the stack trace or error you received? It might help me understand if it's related to php 8.4 or cakeph/orm 5.3.

"php": ">=8.1",
"cakephp/orm": "^5.0"
"php": ">=8.4",
"cakephp/orm": "^5.3.0"
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it should be necessary to increase the requirements. I'd still expect 5.0 users to be able to use this version. If it's required I'll have to publish a new major version of this package.

public function &__get(string $field): mixed
{
return $this->get($field);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Cake has always had a __get() method on the EntityTrait, see:

4.x: https://github.com/cakephp/cakephp/blob/4.x/src/Datasource/EntityTrait.php#L139-L148
5.x: https://github.com/cakephp/cakephp/blob/5.x/src/Datasource/EntityTrait.php#L146-L155

It looks like what has changed is that it now calls getRequiredOrFail(). I'm struggling to understand how this fixed your issue!

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