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

Rename DataObject method name #10637

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Jan 10, 2023

@sabina-talipova sabina-talipova force-pushed the pulls/5/checking-parent-private-methods branch from 31df80e to c9e203a Compare January 10, 2023 02:41
@sabina-talipova sabina-talipova changed the title Check is_callable parent methods before invoke Rename DataObject method name Jan 10, 2023
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

We should not be renaming this method because of the failure on a totally unrelated module - that won't stop the same issue happening again on a different module

Instead we should be using is_callable() as mentioned here

@sabina-talipova
Copy link
Contributor Author

Instead we should be using is_callable() as mentioned here

Using this approach, some "setProperty" methods in another classes fail Unit tests. Initially I implemented this approach, but after testing I think better to rename method and open issue to fix problem with private methods.
Please have a look: 31df80e
and
https://github.com/silverstripe/silverstripe-framework/actions/runs/3879244028/jobs/6616354722

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

In ViewableData::__set() it should be is_callable([$this, $method]), not is_callable($this, $method);

@sabina-talipova sabina-talipova force-pushed the pulls/5/checking-parent-private-methods branch from c9e203a to 9761cef Compare January 11, 2023 00:41
@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Jan 11, 2023

In ViewableData::__set() it should be is_callable([$this, $method]), not is_callable($this, $method);

Just using is_callable, probably doesn't solve the problem, even together with method_exist. I think problem is more deeper and will appear time to time, especially with some Field names.
How I understood, because we have a trait CustomMethods which has magic method __call and we use this trait in trait Extensible , that we use in class ViewableData .
It means if some class extends ViewableData or DataObject and we will have in ViewableData or DataObject classes setSomething methods and extending class has Something field, even if setSomething is private method_exist returns TRUE and is_callable returns TRUE, since CustomMethods has magic method __call .
I've done some tests and is_callable returns TRUE for setFieldValue in EditableCustomRule class.

@sabina-talipova sabina-talipova force-pushed the pulls/5/checking-parent-private-methods branch 2 times, most recently from 0f722f5 to cad30c1 Compare January 11, 2023 01:52
Copy link
Contributor

@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.

There probably should be a unit test to cover this edge case.

Are we happy with the magic setter accessing protected method?

@sabina-talipova
Copy link
Contributor Author

@emteknetnz said that we should check that method is NOT private. Public or protected are fine.

@sabina-talipova sabina-talipova force-pushed the pulls/5/checking-parent-private-methods branch from cad30c1 to da5a902 Compare January 11, 2023 19:22
@sabina-talipova
Copy link
Contributor Author

There probably should be a unit test to cover this edge case.

DONE. New test case was added.

@sabina-talipova sabina-talipova force-pushed the pulls/5/checking-parent-private-methods branch from da5a902 to 638ebcc Compare January 11, 2023 19:27
Copy link
Contributor

@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 6d45425 into silverstripe:5 Jan 11, 2023
@maxime-rainville maxime-rainville deleted the pulls/5/checking-parent-private-methods branch January 11, 2023 20:37
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