Skip to content

Fix Missing getDefaultInstruction() Method

Priority: 🔴 P0 - CRITICAL Status: Planning Related Analysis: phpstan-rector-analysis-overview.md

Problem Statement

RUNTIME BUG: Multiple calls to RoasterCrawlConfig::getDefaultInstruction() method that doesn't exist.

PHPStan Errors

src/Service/Crawler/Strategy/RoasterExtractionStrategyFactory.php:64
  Call to an undefined method App\Entity\RoasterCrawlConfig::getDefaultInstruction()

src/Service/Crawler/Strategy/RoasterExtractionStrategyFactory.php:65
  Call to an undefined method App\Entity\RoasterCrawlConfig::getDefaultInstruction()

src/Service/Crawler/Strategy/CoffeeBeanExtractionStrategyFactory.php:101
  Call to an undefined method App\Entity\RoasterCrawlConfig::getDefaultInstruction()

src/Service/Crawler/Strategy/CoffeeBeanExtractionStrategyFactory.php:102
  Call to an undefined method App\Entity\RoasterCrawlConfig::getDefaultInstruction()

Impact

  • Severity: CRITICAL - Will cause fatal errors when executed
  • Affected Features: Roaster and coffee bean extraction strategies
  • User Impact: Crawler will fail when processing roasters
  • Production Risk: HIGH - May already be causing issues

Root Cause Analysis

Likely Scenarios

  1. Incomplete Refactoring

    • Method was removed from entity but calls not updated
    • Relationship was planned but never implemented

  2. Dead Code

    • Null checks suggest it was expected to return null
    • May have been placeholder for future feature

  3. Missing Relationship

    • RoasterCrawlConfig should have defaultInstruction property
    • Related Instruction entity may exist or be planned

Current Code Pattern

From strategy factories (lines 64-65, 101-102):

if ($config->getDefaultInstruction() !== null) {
    $baseInstructions .= "\n\n" . $config->getDefaultInstruction();
}

Pattern indicates:

  • Expected nullable return value
  • Would append default instruction to base instructions if present
  • Gracefully handles null (so may never have worked)

Investigation Steps

Step 1: Check RoasterCrawlConfig Entity

  • Verify no defaultInstruction property exists
  • Check for any similar properties (instruction, extractionInstructions, etc.)
  • Review entity relationships

Step 2: Search for Instruction Entity/Concept

  • Search for Instruction entity
  • Check if there's instruction-related code elsewhere
  • Review git history for removed instruction code

Step 3: Check Database Schema

  • Verify no default_instruction column exists
  • Check for related instruction tables
  • Review migrations for context

Step 4: Review Git History

  • When was this code added?
  • Was getDefaultInstruction() ever implemented?
  • Any commits removing instruction-related code?

Step 5: Check Strategy Factory Usage

  • How often are these factories called?
  • Are errors occurring in production logs?
  • What's the current behavior?

Proposed Solutions

Assumption: This was always intended to return null, feature never implemented

Changes:

// RoasterExtractionStrategyFactory.php:64-65
// REMOVE these lines entirely:
// if ($config->getDefaultInstruction() !== null) {
//     $baseInstructions .= "\n\n" . $config->getDefaultInstruction();
// }

// CoffeeBeanExtractionStrategyFactory.php:101-102
// REMOVE these lines entirely

Pros:

  • Simple, no database changes
  • Fixes error immediately
  • No behavioral change (was always null anyway)

Cons:

  • Removes potential future feature

Option B: Use Existing extractionInstructions Property

Assumption: extractionInstructions already serves this purpose

Changes:

// Check if RoasterCrawlConfig has extractionInstructions property
if ($config->getExtractionInstructions() !== null) {
    $baseInstructions .= "\n\n" . $config->getExtractionInstructions();
}

Pros:

  • Uses existing data
  • May be what was originally intended

Cons:

  • Need to verify this is semantically correct

Option C: Add Missing Property/Relationship

Assumption: Feature was planned but never finished

Changes:

  1. Add defaultInstruction property to RoasterCrawlConfig
  2. Create migration
  3. Update admin interface to allow setting
  4. Keep existing code as-is

Pros:

  • Enables intended feature
  • No code removal

Cons:

  • Most effort
  • Need to understand business requirements
  • Database migration required

Option D: Make It Nullable Return with Default

Assumption: Safest approach while investigating

Changes:

// Add method to RoasterCrawlConfig entity
public function getDefaultInstruction(): ?string
{
    return null; // or return some default value
}

Pros:

  • Fixes PHPStan error immediately
  • Preserves existing code structure
  • Minimal change

Cons:

  • Doesn't remove dead code
  • Adds method that may not be needed

Start with Option A (Remove Dead Code) UNLESS investigation reveals:

  • Property exists in database
  • Production data uses this feature
  • Recent git history shows this was removed accidentally

Reasoning:

  • Null checks suggest it never worked
  • Simplest solution
  • Removes confusion
  • Can always add back later if needed

Implementation Plan

Step 1: Investigation (1-2 hours)

  1. Read RoasterCrawlConfig entity
  2. Check database schema
  3. Review git history
  4. Check production logs for related errors

Step 2: Decision

Based on investigation, choose solution option

Step 3: Implementation (30 min - 2 hours depending on option)

For Option A (Remove Dead Code):

  1. Remove lines from RoasterExtractionStrategyFactory.php:64-65
  2. Remove lines from CoffeeBeanExtractionStrategyFactory.php:101-102
  3. Run PHPStan to verify fix
  4. Test strategy factories

For Option B (Use extractionInstructions):

  1. Verify extractionInstructions exists and is appropriate
  2. Update both factory files
  3. Test with existing configs
  4. Run PHPStan

For Option C (Add Property):

  1. Add property to entity
  2. Create migration
  3. Update admin interface
  4. Test full flow
  5. Run PHPStan

Step 4: Testing

  • Unit test both strategy factories
  • Integration test crawler flow
  • Verify no production errors
  • Run full PHPStan suite

Step 5: Documentation

  • Update entity documentation if property added
  • Add comment explaining why code was removed (if applicable)

Success Criteria

  • PHPStan errors for getDefaultInstruction() resolved (4 errors → 0)
  • No runtime errors in strategy factories
  • Crawler continues to function correctly
  • Clear code without confusing dead code (if removed)

Risk Assessment

Low Risk IF Option A:

  • Code was never working
  • Removing dead code is safe
  • No functional change

Medium Risk IF Option B:

  • Need to verify semantic correctness
  • Could change behavior if wrong property

High Risk IF Option C:

  • Database migration required
  • New feature needs testing
  • Admin UI changes

Estimated Effort

  • Investigation: 1-2 hours
  • Implementation (Option A): 30 minutes
  • Implementation (Option B): 1 hour
  • Implementation (Option C): 4-6 hours
  • Testing: 1-2 hours

Total: 2.5-10 hours depending on option

Dependencies

None - can be fixed immediately

Follow-up

If Option A is chosen:

  • Document that default instructions feature was never implemented
  • Create ticket for future if feature is desired
  • Clean up any related dead code

If Option C is chosen:

  • Update user documentation
  • Train users on new feature
  • Monitor usage

Notes

  • This is blocking deployment to stricter PHPStan levels
  • Fix should be done before any strategy factory refactoring
  • Check if related to recent multi-market changes
  • May be related to RoasterCrawlConfig having multiple instruction fields that are confusing