fix: surface silently skipped custom validators in the log - #313
Open
Ibochkarev wants to merge 1 commit into
Open
fix: surface silently skipped custom validators in the log#313Ibochkarev wants to merge 1 commit into
Ibochkarev wants to merge 1 commit into
Conversation
When a &validate entry names a type that isn't a built-in method and isn't listed in customValidators, Validator::validate() skips it and treats the field as valid with no error message. That's by design (customValidators is an allowlist to stop brute-forcing arbitrary snippets), but the only trace was a LOG_LEVEL_INFO log entry, which sits below MODX's default log_level and never shows up. From the outside this looks exactly like "the custom validator's error message isn't showing", including for validators like reCAPTCHA that run through the same customValidators-gated snippet path. Bump that log line to LOG_LEVEL_WARN and spell out the fix (add the type to &customValidators) so the actual cause is visible without digging into FormIt's source. Also hardened the customValidators normalization above it: $this->config['customValidators'] can arrive as an array (the constructor already explodes it from formit->config) or as a raw string (when the config passed to the Validator's constructor carries the unprocessed scriptProperty). The old code always called explode() on it, which throws a TypeError if it's already an array. Reproduced both paths with a standalone script exercising Validator::validateFields()/validate() against a stub custom validator snippet. Fixes Sterc#273
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Validator::validate()requires any validator that isn't a built-in method (custom snippet validators, and anything like reCAPTCHA that runs through the same path) to be listed in&customValidators. That's intentional: it stops a&validateentry from brute-forcing arbitrary snippets on the site.When a validator isn't on that list, FormIt skips it and treats the field as valid, with zero error message. The only trace is a
LOG_LEVEL_INFOlog line, which sits below MODX's defaultlog_leveland never actually gets written. From the template author's side, this looks identical to "my custom validator's error message isn't showing", because nothing tells them the validator never ran.Reproduced the full path with a standalone script that stubs
modX/modSnippetand exercises the realValidator::validateFields()/validate()against the docs' custom-validator example ($validator->addError($key, ...)+ return a bool). WithcustomValidatorsunset,fi.error.<field>stays empty and nothing in the default MODX log explains why. WithcustomValidatorscorrectly set, the same snippet populates the placeholder as expected, so the addError/placeholder plumbing itself is fine, only the "why did nothing happen" signal was missing.Bumped that log call to
LOG_LEVEL_WARNand spelled out the fix in the message (add the type to&customValidators).While in there, also hardened the
customValidatorsnormalization a few lines up:$this->config['customValidators']can arrive as an array (the constructor already explodes it fromformit->config) or as a raw string (when the config passed into the Validator's constructor still carries the unprocessed scriptProperty, which overrides the constructor's array default viaarray_merge). The old code unconditionally calledexplode()on it, which throws aTypeErrorif it's already an array. Now it only explodes when it's actually a string.Fixes #273