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

Add missing type casting interpreter definition #392

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

bramstroker
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no

TypeCastingInterpreter was missing from services.yml.

@bramstroker
Copy link
Contributor Author

bramstroker commented Jul 20, 2023

Changed to draft. Also need to add configuration form still I see to allow user to choose the type to cast to.

I need this interpreter in our project because of the strict typing which has been added to Pimcore 11 class getters and setters.

Currently I'm using the map interpreter and objectbrick setter.
But the map interpreter returns a string and the setter expects an integer. So the solution I'm implementing now is to use the nested interpreter with map and type_casting.

Might be a cleaner solution to let the objectbrick setter detect the fieldType automatically and do the correct type casting accordingly when needed. Thoughts?

@bramstroker bramstroker marked this pull request as draft July 20, 2023 06:09
@bramstroker bramstroker marked this pull request as ready for review July 21, 2023 07:01
@bramstroker
Copy link
Contributor Author

This is working now on my installation now. Had some object fields in some imports which expected a float or int type. But all values extracted from the CSV are strings obviously. This will cause fatal errors in PHP due to strict type hints on the object setters.
Adding the type casting interpreter for these cases solved my problem.

@bramstroker
Copy link
Contributor Author

@dpfaffenbauer Sorry for pinging, but any chance this can be merged on short term?

@dpfaffenbauer
Copy link
Collaborator

@bramstroker thanks for pinging, I somehow missed it.

@dpfaffenbauer dpfaffenbauer merged commit c1a4137 into instride-ch:next Aug 3, 2023
5 checks passed
@dpfaffenbauer
Copy link
Collaborator

thanks for the contribution

@bramstroker
Copy link
Contributor Author

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants