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

BaseOptionsField::EVENT_SET_INPUT_OPTIONS #12221

Merged
merged 2 commits into from
Nov 19, 2022

Conversation

friartuck6000
Copy link
Contributor

Adds an event, and a slight change to surrounding code, to option fields. This allows plugins and modules to modify the options that are rendered for the input.

Related discussion: #12212

@friartuck6000 friartuck6000 requested a review from a team as a code owner October 28, 2022 18:42
@brandonkelly
Copy link
Member

Seems like this could have been simplified if the event was added to BaseOptionsField::options() instead?

@friartuck6000
Copy link
Contributor Author

That was my first thought, but BaseOptionsField::options() is used for other things besides generating the input HTML. My goal was to keep this event scoped to only affecting input markup, the way EVENT_SET_FIELD_BLOCK_TYPES is for Matrix.

@brandonkelly
Copy link
Member

Ah ok. In that case it could be added to translatedOptions() which is already effectively the same as inputOptions(). And we can rename translatedOptions() to inputOptions() in Craft 5.

@friartuck6000
Copy link
Contributor Author

The difference is that translatedOptions() doesn't get passed the $value or $element args from inputHtml(), which are a key part of making the event useful. We would have to change the signature of translatedOptions() to accept those. I guess that wouldn't technically be a breaking change since it's not a public method, but it still kinda feels like one, since plugins and user modules could still be using or overriding it.

@brandonkelly brandonkelly changed the base branch from develop to 4.4 November 19, 2022 00:03
@brandonkelly brandonkelly merged commit 94f4ba8 into craftcms:4.4 Nov 19, 2022
@brandonkelly
Copy link
Member

brandonkelly commented Nov 19, 2022

Adding a new $options argument to the beginning of translatedOptions() would have been a worse breaking change than adding new $value and $element arguments after $encode.

So just dropped inputOptions() and the $option argument from translatedOptions(), and added $value and $element, and merged for Craft 4.4 🎉

(Merged via #12351)

@friartuck6000
Copy link
Contributor Author

Heck of a point 🤦‍♂️ thanks!

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