Skip to content

Validation issue refactoring (5961) - #4151

Merged
stracker-phil merged 16 commits into
dev/PCP-4891-agentic-commercefrom
dev/PCP-5961-validation-issue-context
Mar 26, 2026
Merged

Validation issue refactoring (5961)#4151
stracker-phil merged 16 commits into
dev/PCP-4891-agentic-commercefrom
dev/PCP-5961-validation-issue-context

Conversation

@stracker-phil

@stracker-phil stracker-phil commented Mar 6, 2026

Copy link
Copy Markdown
Collaborator

Description

Refactors the validation layer for the Store Sync module around three base classes: ValidationIssue, IssueContext, and ResolutionOption:

Builder Pattern

All three classes use a fluent builder pattern instead of constructor arguments:

ValidationIssue::create_coupon_invalid( 'Coupon expired' )
    ->user_message( 'This coupon is no longer valid.' )
    ->for_field( 'coupons[0]' )
    ->add_context( CouponErrorContext::create_coupon_expired() )
    ->add_resolution(
        ResolutionOption::create_remove_coupon()
            ->label( 'Remove coupon' )
            ->priority( Priority::HIGH )
    );

New IssueContext

Introduces a typed IssueContext hierarchy replacing previously untyped dynamic arrays. Six concrete category classes (ShippingErrorContext, CouponErrorContext, etc.) each expose named factory methods for their specific issue variants:

ShippingErrorContext::create_shipping_to_po_box_not_allowed()
    ->restricted_items( $item_ids )
    ->restriction_reason( 'signature_required' );

ResolutionOption moved and refactored

ResolutionOption moved from Schema/ into Validation/Resolution/. It also has the fluent builder pattern and simplified factory methods:

ResolutionOption::create_update_address()->label( '...' )->priority( Priority::HIGH );
ResolutionOption::create_remove_item()->label( '...' );

ValidationIssue refactored

ValidationIssue is now a single concrete class. Named factory methods replace the previous abstract/child class pattern, with documented "When to use" guidance per variant:

ValidationIssue::create_price_mismatch( 'Price changed' );
ValidationIssue::create_item_out_of_stock( 'No stock remaining' );
ValidationIssue::create_missing_field( 'Shipping address required' );

@stracker-phil stracker-phil changed the title Dev/pcp 5961 validation issue context Validation issue refactoring (5961) Mar 6, 2026

@Narek13 Narek13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @stracker-phil.
A question: did you consider adding a factory instead of these many classes?

class BusinessRuleFactory {
	public function create_rule( string $type ): BusinessRuleErrorIssue
	{
		return new BusinessRuleErrorIssue( $type );
	}
}

////consuming
$business_rule_factory = new BusinessRuleFactory();
$business_rule_factory->create_rule( ContextBusinessRuleIssue::CUSTOMER_ACCOUNT_SUSPENDED );

Or can you please explain why can't we group them under some enum or interface?

@stracker-phil

Copy link
Copy Markdown
Collaborator Author

@Narek13 great question, and the decision to use classes instead of a property is intentional at the time of creating the PR, my thoughts were:

Downside of enum passed via constructor

  • Can potentially receive any string (at least in php 7.4)
  • It's very easy to drift from enum to magic strings, which classes avoid

Benefits of classes:

  • Type safety; it's impossible to create an invalid context
  • Code inspection (which Type is used where)

But honestly, the number of extra files does not really provide real benefits. I will remove those classes again and replace them with an enum, or specific create methods, like BusinessRuleErrorContext::create_minimum_order_not_met()

This would be good case for an ADR or internal follow-up discussion.

@stracker-phil
stracker-phil merged commit 99683e5 into dev/PCP-4891-agentic-commerce Mar 26, 2026
10 of 20 checks passed
@stracker-phil
stracker-phil deleted the dev/PCP-5961-validation-issue-context branch March 26, 2026 17:13
@stracker-phil
stracker-phil restored the dev/PCP-5961-validation-issue-context branch April 29, 2026 12:37
@stracker-phil
stracker-phil deleted the dev/PCP-5961-validation-issue-context branch April 29, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants