Improve post type registration logic#269
Conversation
Fix undefined property warnings and improve data validation in
register_emulated_post_types() method.
This commit addresses several issues:
1. **Fixed undefined property warnings**: Previously, the code
attempted to access $pt->post_type and $pt->label properties
without verifying that the data structure was valid, causing
"Undefined property: stdClass::$post_type" warnings when
corrupted data existed in the database.
2. **Added automatic data cleanup**: The method now automatically
cleans up corrupted entries (non-array items or entries missing
required keys) and saves the cleaned data back to the database.
3. **Improved validation**: Added proper validation to ensure that:
- Each entry is a valid array
- Required keys (post_type and label) exist
- Empty values are allowed for new entries (to support the UI
workflow where users add empty entries first, then fill them in)
- Only entries with non-empty values are registered as post types
4. **Enhanced security**: Added sanitization using sanitize_key()
and sanitize_text_field() for all user input before processing
and registration.
5. **Better error handling**: The method now gracefully handles
corrupted data by skipping invalid entries instead of throwing
errors, allowing users to delete post types without issues.
The fix ensures that:
- Users can add new post types without errors
- Users can delete post types without warnings
- Corrupted data is automatically cleaned up
- The registration process only attempts to register valid,
non-empty post types
Fixes issues where users were unable to delete post types after
adding them, and prevents undefined property warnings in the
debug log.
|
Caution Review failedThe pull request is closed. WalkthroughReplaces direct emulated post type registration with validation and cleaning: early return if input missing, normalize entries to arrays, require and sanitize Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Limits as class-post-type-limits
participant Settings
Caller->>Limits: register_emulated_post_types()
Limits->>Limits: read emulated_post_types
alt not a non-empty array
Limits-->>Caller: return (no registration)
else array
Limits->>Limits: for each entry
Limits->>Limits: ensure entry is array
Limits->>Limits: check required keys (post_type,label)
Limits->>Limits: sanitize post_type & label
alt any entries cleaned/invalid
Limits->>Settings: save cleaned emulated_post_types
end
Limits->>Limits: filter out empty/invalid entries
loop register valid types
Limits->>Limits: if not already registered -> register_post_type()
end
Limits-->>Caller: done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
inc/limits/class-post-type-limits.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
inc/limits/class-post-type-limits.php (1)
inc/functions/settings.php (1)
wu_save_setting(51-54)
🪛 GitHub Check: Code Quality Checks
inc/limits/class-post-type-limits.php
[failure] 97-97:
Expected 1 space after "!"; 0 found
[failure] 97-97:
Expected 1 space after "!"; 0 found
[failure] 91-91:
Expected 1 space after "!"; 0 found
[warning] 87-87:
Equals sign not aligned with surrounding assignments; expected 7 spaces but found 1 space
[failure] 81-81:
Expected 1 space after "!"; 0 found
🪛 PHPMD (2.15.0)
inc/limits/class-post-type-limits.php
89-89: Avoid unused local variables such as '$index'. (undefined)
(UnusedLocalVariable)
⏰ 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). (2)
- GitHub Check: cypress (8.2, chrome)
- GitHub Check: cypress (8.1, chrome)
🔇 Additional comments (1)
inc/limits/class-post-type-limits.php (1)
138-149: Post type registration arguments are appropriate for emulated post types.Only one
register_post_type()call exists in the codebase. The settings correctly configure the post type as internal and non-discoverable: public visibility withshow_in_menuandexclude_from_searchdisabled prevents admin menu clutter, whilehas_archiveandcan_exportset to false align with internal-only usage. Thedelete_with_userflag set to false correctly preserves system post type integrity.
| $cleaned_post_types = []; | ||
| $needs_update = false; | ||
|
|
||
| foreach ($emulated_post_types as $index => $pt) { |
There was a problem hiding this comment.
Remove unused loop variable.
The $index variable is declared but never used in the loop body.
Apply this diff:
- foreach ($emulated_post_types as $index => $pt) {
+ foreach ($emulated_post_types as $pt) {Based on static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach ($emulated_post_types as $index => $pt) { | |
| foreach ($emulated_post_types as $pt) { |
🧰 Tools
🪛 PHPMD (2.15.0)
89-89: Avoid unused local variables such as '$index'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In inc/limits/class-post-type-limits.php around line 89, the foreach declares an
unused $index variable; remove it so the loop reads foreach
($emulated_post_types as $pt) (or use a throwaway variable) to eliminate the
unused variable warning and keep the loop body unchanged.
| $cleaned_post_types[] = [ | ||
| 'post_type' => isset($pt['post_type']) ? sanitize_key($pt['post_type']) : '', | ||
| 'label' => isset($pt['label']) ? sanitize_text_field($pt['label']) : '', | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant isset checks.
Lines 104-105 check isset($pt['post_type']) and isset($pt['label']) again, but line 97 already verified these keys exist. The ternary operators are unnecessary here.
Apply this diff to simplify:
// Add data (even if empty, to allow users to fill in new post types)
$cleaned_post_types[] = [
- 'post_type' => isset($pt['post_type']) ? sanitize_key($pt['post_type']) : '',
- 'label' => isset($pt['label']) ? sanitize_text_field($pt['label']) : '',
+ 'post_type' => sanitize_key($pt['post_type']),
+ 'label' => sanitize_text_field($pt['label']),
];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In inc/limits/class-post-type-limits.php around lines 103 to 106, the array
assignment redundantly checks isset($pt['post_type']) and isset($pt['label')
even though those keys were validated earlier (line ~97); remove the ternary
isset checks and directly use sanitize_key($pt['post_type']) and
sanitize_text_field($pt['label']) when building $cleaned_post_types so the code
is simpler and still safe given the prior validation.
| // Save cleaned data if there were any changes | ||
| if ($needs_update) { | ||
| wu_save_setting('emulated_post_types', $cleaned_post_types); | ||
| $emulated_post_types = $cleaned_post_types; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider verifying save operation success.
The code continues with $cleaned_post_types regardless of whether wu_save_setting() succeeds. If the save fails silently, corrupted data will be cleaned in memory but persist in the database, requiring repeated cleaning on each request.
Consider adding error handling or logging:
if ($needs_update) {
$saved = wu_save_setting('emulated_post_types', $cleaned_post_types);
if ($saved) {
$emulated_post_types = $cleaned_post_types;
} else {
// Log error or handle failure
}
}Check if wu_save_setting returns a boolean indicating success:
🏁 Script executed:
#!/bin/bash
# Description: Check the return value and behavior of wu_save_setting
# Find the implementation of wu_save_setting
ast-grep --pattern 'function wu_save_setting($$$) {
$$$
}'
# Also check the settings class save_setting method
ast-grep --pattern 'function save_setting($$$) {
$$$
}'Length of output: 230
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Find the save_setting method implementation in the settings class
# Search for the save_setting method definition
ast-grep --pattern 'save_setting($$$) {
$$$
}'
# Also search more broadly for this method
rg -n "save_setting" --type=php -B 2 -A 10 | head -100
Length of output: 9800
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Find the Settings class and save_setting method implementation
# Search for class Settings
rg -n "class Settings" --type=php -A 2
# Search for the save_setting function definition (not calls)
rg -n "function save_setting\(" --type=php -A 15 | head -150
Length of output: 2154
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Get the complete save_setting method with return statement
rg -n "public function save_setting" --type=php -A 25 inc/class-settings.php
Length of output: 1610
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Find wu_save_option implementation to confirm return value
rg -n "function wu_save_option" --type=php -A 15
Length of output: 852
Add return value handling for wu_save_setting() to prevent silent save failures.
The call to wu_save_setting() at line 112 returns a boolean indicating success, but this value is ignored. If the save fails, corrupted data will be cleaned in memory while stale data persists in the database, requiring repeated cleanup on each request.
Update lines 109-113 to check the return value:
if ($needs_update) {
if (wu_save_setting('emulated_post_types', $cleaned_post_types)) {
$emulated_post_types = $cleaned_post_types;
} else {
// Handle save failure
}
}Alternatively, log the failure or trigger an admin notice to alert users of potential data consistency issues.
🤖 Prompt for AI Agents
In inc/limits/class-post-type-limits.php around lines 109 to 113, the call to
wu_save_setting('emulated_post_types', $cleaned_post_types) currently ignores
its boolean return value which can hide save failures and leave the DB stale;
change the block to check the return: only set $emulated_post_types =
$cleaned_post_types when wu_save_setting returns true, and if it returns false
handle the failure (e.g., log an error with relevant context and/or trigger an
admin notice) so callers are informed and data consistency issues are surfaced.
| $post_type = sanitize_key($pt['post_type']); | ||
| $label = sanitize_text_field($pt['label']); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant sanitization.
These lines sanitize post_type and label again, but they were already sanitized in the cleaning loop (lines 104-105). When $needs_update is true, $emulated_post_types is replaced with $cleaned_post_types on line 112, which contains sanitized values. When $needs_update is false, the data passed validation but wasn't modified—yet sanitizing again is still defensive duplication.
Since the empty checks on lines 118-120 already guard against empty values, you can simplify lines 122-128 to:
// Register only valid post types (skip empty ones)
foreach ($emulated_post_types as $pt) {
// Skip if post_type or label is empty
if (empty($pt['post_type']) || empty($pt['label'])) {
continue;
}
- $post_type = sanitize_key($pt['post_type']);
- $label = sanitize_text_field($pt['label']);
-
- // Skip if sanitization resulted in empty values
- if (empty($post_type) || empty($label)) {
- continue;
- }
-
// Check if post type is already registered
- $existing_pt = get_post_type_object($post_type);
+ $existing_pt = get_post_type_object($pt['post_type']);
if ($existing_pt) {
continue;
}
// Register the post type
register_post_type(
- $post_type,
+ $pt['post_type'],
[
- 'label' => $label,
+ 'label' => $pt['label'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $post_type = sanitize_key($pt['post_type']); | |
| $label = sanitize_text_field($pt['label']); | |
| // Register only valid post types (skip empty ones) | |
| foreach ($emulated_post_types as $pt) { | |
| // Skip if post_type or label is empty | |
| if (empty($pt['post_type']) || empty($pt['label'])) { | |
| continue; | |
| } | |
| // Check if post type is already registered | |
| $existing_pt = get_post_type_object($pt['post_type']); | |
| if ($existing_pt) { | |
| continue; | |
| } | |
| // Register the post type | |
| register_post_type( | |
| $pt['post_type'], | |
| [ | |
| 'label' => $pt['label'], |
🤖 Prompt for AI Agents
In inc/limits/class-post-type-limits.php around lines 122-123, the code
redundantly re-sanitizes post_type and label even though $cleaned_post_types
already contains sanitized values; remove the sanitize_key/sanitize_text_field
calls and use the existing $pt['post_type'] and $pt['label'] directly (e.g.
assign $post_type = $pt['post_type']; $label = $pt['label'];), keeping the
existing empty checks on lines 118-120 and the subsequent logic unchanged so
behavior and validation remain identical.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
inc/limits/class-post-type-limits.php (3)
103-106: Simplifycleaned_post_typesconstruction by removing redundantissetchecks.Because the preceding
isset($pt['post_type'])/isset($pt['label'])condition guarantees both keys exist and are non-null, the extraisset()checks in the assignment are never false and can be dropped for clarity.// Add data (even if empty, to allow users to fill in new post types) $cleaned_post_types[] = [ - 'post_type' => isset($pt['post_type']) ? sanitize_key($pt['post_type']) : '', - 'label' => isset($pt['label']) ? sanitize_text_field($pt['label']) : '', + 'post_type' => sanitize_key($pt['post_type']), + 'label' => sanitize_text_field($pt['label']), ];Based on static analysis hints.
89-97: Remove unused$indexand fix!spacing to satisfy coding standards.The loop index is never used and the
!operators are missing the required space. Cleaning these up will also clear the static-analysis warnings.- foreach ($emulated_post_types as $index => $pt) { + foreach ($emulated_post_types as $pt) { // Verify that $pt is an array - if (!is_array($pt)) { + if ( ! is_array($pt)) { $needs_update = true; continue; } // Verify that required keys exist (allow empty values for new entries) - if (!isset($pt['post_type']) || !isset($pt['label'])) { + if ( ! isset($pt['post_type']) || ! isset($pt['label'])) { $needs_update = true; continue; }Based on static analysis hints.
109-113: Consider handlingwu_save_setting()failures instead of ignoring the return value.
wu_save_setting('emulated_post_types', $cleaned_post_types)returns a status from the settings layer, but the result is currently discarded. If the save fails, the database keeps corrupted entries while this request proceeds with cleaned data, and the cleanup will be repeated on every request without any visibility.You can at least surface failures while preserving current behavior:
// Save cleaned data if there were any changes if ($needs_update) { - wu_save_setting('emulated_post_types', $cleaned_post_types); - $emulated_post_types = $cleaned_post_types; + if ( ! wu_save_setting('emulated_post_types', $cleaned_post_types) ) { + // Optional: log or otherwise handle a failed settings save here. + } + + $emulated_post_types = $cleaned_post_types; }This keeps registration using the cleaned entries while giving you a hook to diagnose persistence issues when the settings write fails.
🧹 Nitpick comments (1)
inc/limits/class-post-type-limits.php (1)
85-107: Optional: consider always using$cleaned_post_typesfor registration.You already build
$cleaned_post_typesfor every valid entry, but when$needs_updateisfalseyou continue to feed the registration loop from the original$emulated_post_types. You could unconditionally use$cleaned_post_typesfor registration (keeping$needs_updateonly to decide whether to persist back to the DB) to avoid re-reading raw settings and to centralize use of the validated data. Not required for correctness, but worth considering for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
inc/limits/class-post-type-limits.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
inc/limits/class-post-type-limits.php (1)
inc/functions/settings.php (1)
wu_save_setting(51-54)
🪛 GitHub Check: Code Quality Checks
inc/limits/class-post-type-limits.php
[failure] 97-97:
Expected 1 space after "!"; 0 found
[failure] 97-97:
Expected 1 space after "!"; 0 found
[failure] 91-91:
Expected 1 space after "!"; 0 found
🪛 PHPMD (2.15.0)
inc/limits/class-post-type-limits.php
89-89: Avoid unused local variables such as '$index'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (2)
inc/limits/class-post-type-limits.php (2)
81-83: Guard clause for empty/invalid settings is appropriate.Short-circuiting when
emulated_post_typesis not a non-empty array avoids unnecessary work and prevents downstream access on unexpected types; this matches the intended “do nothing if nothing configured” behavior.
115-149: Registration loop correctly filters and registers only valid, non-duplicate post types.The second pass over
$emulated_post_types:
- skips entries with empty
post_typeorlabel,- sanitizes both fields before use,
- avoids re-registering existing post types via
get_post_type_object(), and- calls
register_post_type()with the expected options for these emulated types.This matches the PR goal of registering only valid, non-empty, non-duplicate emulated post types.
|
@tamkendigital Thanks for the PR! It works perfectly. I just made some code style changes and merged it. It'll be in the next release. |
2 participants
Fix undefined property warnings and improve data validation in register_emulated_post_types() method.
This commit addresses several issues:
Fixed undefined property warnings: Previously, the code attempted to access $pt->post_type and $pt->label properties without verifying that the data structure was valid, causing "Undefined property: stdClass::$post_type" warnings when corrupted data existed in the database.
Added automatic data cleanup: The method now automatically cleans up corrupted entries (non-array items or entries missing required keys) and saves the cleaned data back to the database.
Improved validation: Added proper validation to ensure that:
Enhanced security: Added sanitization using sanitize_key() and sanitize_text_field() for all user input before processing and registration.
Better error handling: The method now gracefully handles corrupted data by skipping invalid entries instead of throwing errors, allowing users to delete post types without issues.
The fix ensures that:
Fixes issues where users were unable to delete post types after adding them, and prevents undefined property warnings in the debug log.
Summary by CodeRabbit