Fix url form field #1439
Fix url form field #1439
Conversation
mjauvin
added
the
maintenance
WalkthroughRemoved default from FormField::$size, added magic __get/__isset for config-backed properties, auto-size behavior for textarea/widget in displayAs, and reorganized which config keys are applied. URL field partial now conditionally renders autocomplete, maxlength, minlength, pattern, placeholder, and numeric size; removed required/readonly/disabled rendering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/backend/classes/FormField.php`:
- Around line 742-751: The __get magic method can call array_key_exists($name,
$this->config) when $this->config is null causing a TypeError; make this
null-safe by first ensuring $this->config is an array (either initialize
$this->config = [] in the constructor or coerce before checking) and replace the
existing check with something like if (is_array($this->config) &&
array_key_exists($name, $this->config)) or use $config = (array)$this->config
and then array_key_exists($name, $config); keep the subsequent
array_get($this->config, $name) consistent with the chosen approach so you don’t
pass null into array helpers.
- Around line 757-763: The __isset method can throw a TypeError if $this->config
is null because array_key_exists expects an array; update the check to first
ensure $this->config is an array (e.g., use is_array($this->config) &&
array_key_exists($name, $this->config)) before calling array_key_exists, keeping
the existing fallback to property_exists($this, $name) &&
!is_null($this->{$name}) to preserve behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/backend/classes/FormField.phpmodules/backend/widgets/form/partials/_field_url.php
🧰 Additional context used
🧬 Code graph analysis (1)
modules/backend/widgets/form/partials/_field_url.php (1)
modules/backend/classes/FormField.php (1)
size(217-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: windows-latest / JavaScript
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.2
🔇 Additional comments (5)
modules/backend/classes/FormField.php (3)
94-96: LGTM - Size property no longer has a universal default.Removing the default
'large'value and compensating indisplayAs()for textarea/widget types is a sensible approach. This allows other field types to have no size unless explicitly configured.
262-266: LGTM - Appropriate default for textarea and widget types.Setting
size = 'large'before the type is assigned ensures textarea and widget fields maintain their expected sizing behavior after removing the property-level default.
289-300: LGTM - Config keys reorganized.The expanded list of auto-applied config values (
context,cssClass,placeholder,preset,readOnly,required,stretch,trigger) centralizes configuration handling appropriately.modules/backend/widgets/form/partials/_field_url.php (2)
33-33: LGTM - Autocomplete attribute properly rendered.Leveraging
isset($field->autocomplete)(via the new__issetmagic method) to conditionally render the autocomplete attribute is the right approach.
37-37: LGTM - Size attribute restricted to numeric values.The HTML
sizeattribute expects a numeric value, whileFormField::$sizecan hold string descriptors like'large'or'small'for CSS purposes. This check correctly prevents invalid HTML attributes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| public function __get($name) | ||
| { | ||
| if (array_key_exists($name, $this->config)) { | ||
| return array_get($this->config, $name); | ||
| } | ||
| if (property_exists($this, $name)) { | ||
| return $this->{$name}; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
TypeError when $this->config is null.
array_key_exists() throws a TypeError in PHP 8+ if the second argument is null. Since $config is not initialized in the constructor and only set when displayAs() is called, accessing dynamic properties on a field that hasn't been fully configured will crash.
🐛 Proposed fix
public function __get($name)
{
- if (array_key_exists($name, $this->config)) {
+ if (is_array($this->config) && array_key_exists($name, $this->config)) {
return array_get($this->config, $name);
}
if (property_exists($this, $name)) {
return $this->{$name};
}
return null;
}🤖 Prompt for AI Agents
In `@modules/backend/classes/FormField.php` around lines 742 - 751, The __get
magic method can call array_key_exists($name, $this->config) when $this->config
is null causing a TypeError; make this null-safe by first ensuring $this->config
is an array (either initialize $this->config = [] in the constructor or coerce
before checking) and replace the existing check with something like if
(is_array($this->config) && array_key_exists($name, $this->config)) or use
$config = (array)$this->config and then array_key_exists($name, $config); keep
the subsequent array_get($this->config, $name) consistent with the chosen
approach so you don’t pass null into array helpers.
| public function __isset($name) | ||
| { | ||
| if (array_key_exists($name, $this->config)) { | ||
| return true; | ||
| } | ||
| return property_exists($this, $name) && !is_null($this->{$name}); | ||
| } |
There was a problem hiding this comment.
Same null-safety issue in __isset().
This method has the same TypeError risk when $this->config is null.
🐛 Proposed fix
public function __isset($name)
{
- if (array_key_exists($name, $this->config)) {
+ if (is_array($this->config) && array_key_exists($name, $this->config)) {
return true;
}
return property_exists($this, $name) && !is_null($this->{$name});
}🤖 Prompt for AI Agents
In `@modules/backend/classes/FormField.php` around lines 757 - 763, The __isset
method can throw a TypeError if $this->config is null because array_key_exists
expects an array; update the check to first ensure $this->config is an array
(e.g., use is_array($this->config) && array_key_exists($name, $this->config))
before calling array_key_exists, keeping the existing fallback to
property_exists($this, $name) && !is_null($this->{$name}) to preserve behavior.
FormField now accepts new attributes dynamically.
Summary by CodeRabbit
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.