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

DEP PHP Support in CMS5 #1182

Merged

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Jan 9, 2023

@emteknetnz , there are some issues with recent changes to DataObject to support php 8.2 https://github.com/silverstripe/silverstripe-framework/blob/c1a773310d088b22bea7ae1a6e1930050f0a33e2/src/ORM/DataObject.php#L2854

I would like to clarify whether we can change the access modifier from private to protected for setFieldValue method, since those classes that inherit DataObject and have this FieldValue field try to call a method to which they do not have access?

Thank you in advance :)

@emteknetnz
Copy link
Member

I think that's probably the wrong solution, looks like it's coming from this in framework

ViewableData

    public function __set($property, $value)
    {
        $this->objCacheClear();
        if ($this->hasMethod($method = "set$property")) { // <<< here
            $this->$method($value);
        } else {
            $this->setField($property, $value);
        }
    }

I'm asumming it's trying to set the property 'FieldValue' and it seems the 'setFieldValue' method on DataObject and trys to use that, though that method does something different

I think we need to add an an && isAccessible($method) check in ViewableData::__set() for this edge case

Can you confirm that this unit test is trying to set a $property of 'FieldValue' and then try updating ViewableData::__set() on your local?

@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Jan 10, 2023

Can you confirm that this unit test is trying to set a $property of 'FieldValue' and then try updating ViewableData::__set() on your local?

@emteknetnz, if you check this line in GA tests https://github.com/silverstripe/silverstripe-userforms/actions/runs/3870193575/jobs/6596921118#step:12:75 , you will see that tests are failed exactly in this if condition in ViewableData class in public function __set($property, $value) method.
https://github.com/silverstripe/silverstripe-framework/blob/0d3953ba3b016b7343edc7369ddc88c54e7755c0/src/View/ViewableData.php#L158-L159.
So it means it runs setFieldValue method that doesn't exist or accessible in class that extends DataObject.
It's probably, something weird, but method_exists function returns true for private parent methods in DataObject.
But it shouldn't https://www.php.net/manual/en/function.method-exists.php.
But is_callable returns false for private methods.
Probably, something wrong in this note https://www.php.net/manual/en/function.method-exists.php#124462

@emteknetnz
Copy link
Member

Try adding is_callable() in ViewableData::__set() locally - if that works then open up a PR to framework

@sabina-talipova
Copy link
Contributor Author

Try adding is_callable() in ViewableData::__set() locally - if that works then open up a PR to framework

Yes, it does.

@sabina-talipova
Copy link
Contributor Author

Passed tests in local env. Requires PR silverstripe/silverstripe-framework#10637

@sabina-talipova
Copy link
Contributor Author

Failed Behat tests are related to the issue silverstripe/silverstripe-framework#10645

Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

LGTM

@maxime-rainville maxime-rainville merged commit 2024622 into silverstripe:6 Jan 12, 2023
@maxime-rainville maxime-rainville deleted the pulls/6/upgrade-cms5 branch January 12, 2023 21:17
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.

3 participants