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

Call static method with static keyword instead of self in DefaultValueBinder #735

Closed
wants to merge 1 commit into from

Conversation

midlan
Copy link
Contributor

@midlan midlan commented Oct 23, 2018

When I extended DefaultValueBinder and used my custom value binder, I overriden static method dataTypeForValue to examine all cells as strings. But my method is not called, because in DefaultValueBinder it is called with self keyword. Since the class is designed to be extended (not final), I think the use of self keyword is bad design, and static keyword should be used. When used with static, my overriden method dataTypeForValue is called properly.

Maybe same problem exists in other classes in project, but I only discovered this one.

@PowerKiKi
Copy link
Member

Not sure about this. You can easily work around it by overriding bindValue() and possibly bypass dataTypeForValue() entirely.

@MarkBaker any opinion on this ?

@MarkBaker
Copy link
Member

The easiest if your extending binder overrides is to call self::dataTypeForValue() rather than parent::dataTypeForValue(); but there's no reason why the base bindValue() shouldn't use late static binding... though it does mean that any extending class has to ensure that ll edge cases are covered by its own implementation

@midlan
Copy link
Contributor Author

midlan commented Oct 30, 2018

@PowerKiKi I would like to keep bindValue method unchanged (inherited) for possible future updates. I only wanted change the PHP internal types to document types mapping. Which is done in dataTypeForValue method, so it is the only method I wanted override. Now I need override both methods, to workaround (as you said) it. I thinked, when I need to workaround something, it means that something is wrong.

@MarkBaker I dont understand yours

it does mean that any extending class has to ensure that ll edge cases are covered by its own implementation

Calling method with static doesn't mean the method must be implemented in it's childs. When calling method with static keyword, the parent method is called when local overriden is not implemented. Let's look on my example: https://www.tehplayground.com/kKvtHVz7qOLVk8PB
(the C class does not contain print method)

@MarkBaker
Copy link
Member

I mean that if you extend DefaultValueBinder with your own value binder, and you implement your own version of the dataTypeForValue() method, and that is called from the default bindValue() method, then you must guarantee that your version of dataTypeForValue() will always return a valid DataType... no returning void, null or boolen false in the event of the script dropping through your own logic, no returning an arbitrary value that isn't defined in PhpOffice\PhpSpreadsheet\Cell\DataType

Copy link
Member

@MarkBaker MarkBaker left a comment

Choose a reason for hiding this comment

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

Approved, and once we move to PHP >= 7.1.0 we can modify the binder interface and return type hinting to help enforce this

@PowerKiKi PowerKiKi closed this in 3be06a5 Nov 3, 2018
@PowerKiKi
Copy link
Member

Thanks for the contribution, I merged it

guillaume-ro-fr pushed a commit to guillaume-ro-fr/PhpSpreadsheet that referenced this pull request Jun 12, 2019
This allow to avoid overriding `DefaultValueBinder::bindValue()`

Fixes PHPOffice#735
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants