fix(domain): prevent multiple primary domains per site#278
Conversation
- Track original primary state in Domain model to detect changes - Unset other primary domains when setting a new primary - Ensure consistency across API, admin, and creation flows
📝 WalkthroughWalkthroughMove async cleanup of old primary domains from UI/admin handlers into the Domain model save flow. The model now computes pre-save "was new" status and, after persisting, dispatches Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI/Admin
participant DomainModel as "Domain Model"
participant DB as "Database"
participant AsyncQueue as "Async Queue"
participant Cleanup as "Cleanup Handler"
User->>UI/Admin: create or promote domain (mark primary)
UI/Admin->>DomainModel: set attributes and call save()
DomainModel->>DomainModel: compute was_new = !exists()
DomainModel->>DB: persist domain record
DB-->>DomainModel: success
alt domain is primary AND (was_new OR promoted from non-primary)
DomainModel->>AsyncQueue: dispatch wu_async_remove_old_primary_domains(site_id, new_primary_id)
AsyncQueue-->>Cleanup: run async action
Cleanup->>DB: query old primary domains for site
DB-->>Cleanup: return old primary ids
Cleanup->>DB: unset primary flag on old domains
DB-->>Cleanup: success
end
Cleanup-->>UI/Admin: (async) cleanup completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
inc/models/class-domain.php (1)
494-499: Consider clarifying variable naming for readability.The naming of
$new_domain(line 494) and$was_new(line 498) have opposite semantics:
$new_domain = $this->exists()— true when domain exists$was_new = ! $this->exists()— true when domain does not existThis can be confusing for future maintainers. Consider renaming
$new_domainto$existed_before_savefor consistency with$was_new.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
inc/models/class-domain.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
inc/models/class-domain.php (4)
inc/models/class-site.php (1)
exists(1243-1246)inc/database/engine/class-table.php (1)
exists(65-72)inc/objects/class-billing-address.php (1)
exists(95-98)inc/functions/domain.php (1)
wu_get_domains(35-38)
🪛 GitHub Check: Code Quality Checks
inc/models/class-domain.php
[failure] 307-307:
Expected 1 space after "!"; 0 found
[failure] 534-534:
Whitespace found at end of line
⏰ 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.1, chrome)
- GitHub Check: cypress (8.2, chrome)
🔇 Additional comments (2)
inc/models/class-domain.php (2)
57-64: LGTM!The new
$original_primaryproperty is well-documented and appropriately typed. It correctly enables change tracking for the primary domain state.
542-557: Verify the async action handler exists and handles empty arrays gracefully.The logic for detecting when a domain becomes primary is correct. However, ensure:
- The
wu_async_remove_old_primary_domainsaction handler is registered elsewhere.- The handler gracefully handles an empty array (when no other primaries exist).
inc/models/class-domain.php
Outdated
| if ($this->id > 0 && !isset($this->original_primary)) { | ||
| $this->original_primary = $this->primary_domain; | ||
| } |
There was a problem hiding this comment.
Fix code style: add space after negation operator.
Static analysis flagged a missing space after !.
Apply this diff:
- if ($this->id > 0 && !isset($this->original_primary)) {
+ if ($this->id > 0 && ! isset($this->original_primary)) {🧰 Tools
🪛 GitHub Check: Code Quality Checks
[failure] 307-307:
Expected 1 space after "!"; 0 found
🤖 Prompt for AI Agents
In inc/models/class-domain.php around lines 307 to 309, the negation operator is
missing a space; change the conditional to add a space after the '!' so it reads
with '! isset(...)' (i.e. ensure there is a space between the '!' and the
following token) to satisfy the code style/static analysis.
|
@mahyarrezghi Thanks for the PR. I simplified the implementation using $this->_original['primary_domain'] instead of a new original property. Probably it wasn't obvious that the _original property existed but it's for this type a thing. I also removed the other calls to |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
inc/ui/class-domain-mapping-element.php (1)
607-616: Remove unnecessary query for old primary domains.Lines 607-615 query old primary domains but the result is no longer used for cleanup (as noted in the comment at line 616). This creates unnecessary database overhead. Since the cleanup now happens in
Domain::save(), this query serves no purpose except populating the filter parameter at line 637.Consider whether the
$old_primary_domainsparameter in thewu_make_primary_domain_redirect_urlfilter is actually needed by consumers. If not, remove lines 607-615 entirely.🔎 Verify if any code consumes the $old_primary_domains filter parameter
#!/bin/bash # Search for usages of wu_make_primary_domain_redirect_url filter to see if the 4th parameter is used rg -n -A 10 "apply_filters.*wu_make_primary_domain_redirect_url|add_filter.*wu_make_primary_domain_redirect_url"inc/models/class-domain.php (1)
12-12: Remove unused import.The
use stdClass;import is not used anywhere in this file. The reference tostdClassat line 591 is only in a PHPDoc comment, which doesn't require an import.🔎 Proposed fix
-use stdClass;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
inc/admin-pages/class-domain-list-admin-page.phpinc/models/class-domain.phpinc/ui/class-domain-mapping-element.php
💤 Files with no reviewable changes (1)
- inc/admin-pages/class-domain-list-admin-page.php
🧰 Additional context used
🧬 Code graph analysis (1)
inc/models/class-domain.php (4)
inc/models/class-site.php (1)
exists(1243-1246)inc/database/engine/class-table.php (1)
exists(65-72)inc/objects/class-billing-address.php (1)
exists(95-98)inc/functions/domain.php (1)
wu_get_domains(35-38)
⏰ 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). (4)
- GitHub Check: PHP 7.4
- GitHub Check: cypress (8.2, chrome)
- GitHub Check: Build Plugin for Testing
- GitHub Check: cypress (8.1, chrome)
🔇 Additional comments (2)
inc/ui/class-domain-mapping-element.php (1)
620-637: Well-documented filter addition.The new
wu_make_primary_domain_redirect_urlfilter is well-documented and allows customization of the redirect behavior after making a domain primary. Good addition for extensibility.inc/models/class-domain.php (1)
486-486: Correct implementation of new/existing domain tracking.The
$was_newflag correctly captures whether the domain is being created (before the save occurs). This is the right approach for detecting state changes.
| /* | ||
| * If this domain was just set as primary, unset other primaries for this site. | ||
| */ | ||
| if ($this->primary_domain && ($was_new || (isset($this->_original['primary_domain']) && ! $this->_original['primary_domain']))) { | ||
| $old_primary_domains = wu_get_domains( | ||
| [ | ||
| 'primary_domain' => true, | ||
| 'blog_id' => $this->blog_id, | ||
| 'id__not_in' => [$this->id], | ||
| 'fields' => 'ids', | ||
| ] | ||
| ); | ||
|
|
||
| do_action('wu_async_remove_old_primary_domains', $old_primary_domains); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual code at the specified lines to understand the current state
sed -n '531,545p' inc/models/class-domain.php | cat -nRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 706
🏁 Script executed:
# Check line 429 for the wu_enqueue_async_action reference mentioned in the review
sed -n '425,435p' inc/models/class-domain.php | cat -nRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 503
🏁 Script executed:
# Search for listeners that handle wu_async_remove_old_primary_domains action
rg -n "add_action.*wu_async_remove_old_primary_domains|wu_async_remove_old_primary_domains" --type php -B 2 -A 10Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 1732
🏁 Script executed:
# Check if wu_enqueue_async_action is used with this action elsewhere
rg -n "wu_enqueue_async_action.*wu_async_remove_old_primary_domains|wu_async_remove_old_primary_domains.*wu_enqueue_async_action" --type phpRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 63
🏁 Script executed:
# Search for how _original array is populated in base model
rg -n "class Domain|_original|populate.*original" inc/models/class-domain.php -B 2 -A 5 | head -50Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 659
🏁 Script executed:
# Check the parent class/base model to understand _original initialization
find . -name "*.php" -type f | xargs grep -l "class.*Domain.*extends" | grep -v node_modulesRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 828
🏁 Script executed:
# Find the async_remove_old_primary_domains method implementation
rg -n "function async_remove_old_primary_domains" --type php -A 20Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 1338
🏁 Script executed:
# Search for Base_Model class to understand _original initialization
fd "class-base-model.php" --type fRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 112
🏁 Script executed:
# Check if there's any initialization of _original in Domain model
rg -n "class Domain|_original\s*=" inc/models/class-domain.php -B 2 -A 3 | head -40Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 211
🏁 Script executed:
# Look for where wu_enqueue_async_action is actually used in the codebase for comparison
rg -n "wu_enqueue_async_action" --type php -B 2 -A 5 | head -40Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 2433
🏁 Script executed:
# Examine the actual line 534 more carefully for whitespace/formatting
sed -n '534p' inc/models/class-domain.php | cat -ARepository: Multisite-Ultimate/ultimate-multisite
Length of output: 217
🏁 Script executed:
# Check Base_Model to understand how _original is populated during initialization
head -100 inc/models/class-base-model.php | cat -nRepository: Multisite-Ultimate/ultimate-multisite
Length of output: 2350
🏁 Script executed:
# Look for _original initialization in Base_Model
rg -n "_original\s*=" inc/models/class-base-model.php -B 3 -A 3Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 319
🏁 Script executed:
# Check how _original is set when a model is created/loaded
rg -n "public function __construct|function get_original|_original" inc/models/class-base-model.php -B 2 -A 5 | head -80Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 1622
🏁 Script executed:
# Check if the diff provided in review makes sense by comparing whitespace carefully
sed -n '534p' inc/models/class-domain.php | od -c | head -30Repository: Multisite-Ultimate/ultimate-multisite
Length of output: 121
Consider renaming the action or implementing true async queueing for primary domain updates.
The wu_async_remove_old_primary_domains action at line 544 executes synchronously via do_action(), but its name suggests asynchronous processing. The registered listener in inc/managers/class-domain-manager.php (line 895) directly updates domains without queueing async work. To match the naming convention used elsewhere (e.g., wu_enqueue_async_action() at line 210), either rename the action to reflect synchronous execution or use wu_enqueue_async_action() for truly deferred processing.
2 participants
Summary by CodeRabbit
New Features
Behavior Changes
✏️ Tip: You can customize this high-level summary in your review settings.