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

Validate Default Params Against Config Filters #20563

Closed
mbabker opened this issue May 23, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@mbabker
Copy link
Member

commented May 23, 2018

There are a bunch of PRs where forms are (finally) having type filters applied to them. On top of this, we need to do a scan through our install SQL files and sample data and verify that the data in these respects these filters (this will most predominantly affect boolean and numeric types) so that we don't ship with "1" as the value for a boolean true or numeric 1 going forward.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2018

Need to decide whether default plugin params should be included in SQL files or not.

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2018

If there's already a config in the SQL files then it should stay there, if there isn't we shouldn't add it because the PHP code should be reliably setting default values (matching what's defined in the XML schema) and using those for cases where a given param isn't set.

Long term, we shouldn't really need any params in SQL files and I'd say putting them in there should be an exception rather than a standard because this means the core install isn't using the default form settings for some reason.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2018

Plugin configs don't seem to be needed for core and, given how random they are, they're not much use to 3rd parties either.

What about module configs in #__extensions table? Do they actually serve any purpose?

And what should we use as default value for params in the table? Empty string or empty object ({})? I see a mix of both.

@mbabker

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2018

Plugin configs don't seem to be needed for core and, given how random they are, they're not much use to 3rd parties either.

No, but as someone who favors strict typing, I do think it's better in general if the code and the form are consistent on typing (so a numeric value should always be configured in the form to be stored as an integer or float and converted as such when the param read in whatever PHP code uses it). Same for every parameter in every location, regardless of whether it's used by "external" sources or only used within the extension.

What about module configs in #__extensions table? Do they actually serve any purpose?

Nope.

And what should we use as default value for params in the table? Empty string or empty object ({})? I see a mix of both.

Default should be something that can safely be put through json_decode() since we have code paths that do zero validation and blindly try to decode whatever data is given. null and '' both give syntax errors with json_decode(), so '{}' looks to be a sane default.

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

Can this be closed now as resolved by the pr from @SharkyKZ ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.