Skip to content

feat: Add error logging for bulk operations and model preparation#273

Merged
Jeroen-G merged 10 commits into
Jeroen-G:masterfrom
Omnee-Technologies:feature/add-bulk-operation-error-logging
Mar 29, 2026
Merged

feat: Add error logging for bulk operations and model preparation#273
Jeroen-G merged 10 commits into
Jeroen-G:masterfrom
Omnee-Technologies:feature/add-bulk-operation-error-logging

Conversation

@ert485

@ert485 ert485 commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

Add error logging for bulk operations and model preparation

  • Log Elasticsearch bulk operation errors with detailed context including operation type, index, document ID, and full error chains
  • Add error handling in BulkUpdateOperation.modelToData() to catch and log toSearchableArray() and prepare() method failures
  • Include stack traces and model context in error logs for better debugging
  • Allow bulk operations to continue processing other models when individual models fail
  • Resolves silent failure issues where PHP errors in toSearchableArray() were not visible

Fixes issue where errors in model preparation during indexing would fail silently, making debugging difficult for applications consuming this package.

Solution to the issue described in both #221 and #267

@ert485

ert485 commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

This was based from Explorer 3.15.0, because I my project is currently stuck on Laravel 10.x
UPDATE - I merged master in, and rebased. So now this can target 4.x

I might leave it to someone who has a working project that targets Explorer 4.x to resolve the merge conflict and test it on main

(we could also add a "maintenance-3.x" branch based on 3.15.0 that this could get merged directly into)

@Jeroen-G

Jeroen-G commented Oct 2, 2025

Copy link
Copy Markdown
Owner

Thanks for the pull request! 🙏 To manage expectations, it might take at least a week before I have time to properly review this.
As a first remark, I'd like to use an interfaced service somewhere instead of the facade. That would also make it easier to test and is more in line with the rest of the package (see also my comment in the other PR:
#267 (comment))

…reparation

- Log Elasticsearch bulk operation errors with detailed context including operation type, index, document ID, and full error chains
- Add error handling in BulkUpdateOperation.modelToData() to catch and log toSearchableArray() and prepare() method failures
- Include stack traces and model context in error logs for better debugging
- Allow bulk operations to continue processing other models when individual models fail
- Resolves silent failure issues where PHP errors in toSearchableArray() were not visible

Fixes issue where errors in model preparation during indexing would fail silently,
making debugging difficult for applications consuming this package.
@ert485
ert485 force-pushed the feature/add-bulk-operation-error-logging branch from f71aa46 to d80762f Compare October 3, 2025 17:08
@ert485

ert485 commented Oct 3, 2025

Copy link
Copy Markdown
Contributor Author

@Jeroen-G
The concerns you have brought up are fixed, and it is ready for your review (when you get a chance).

  • PSR3 logging
  • I added tests
  • I got the auto formatter to fix issues, so the workflow that failed should work this time

@ert485

ert485 commented Oct 6, 2025

Copy link
Copy Markdown
Contributor Author

Breaking Changes Summary

This PR introduces breaking changes to several class constructors and method signatures:

Constructor Changes (Required Logger Parameter)

  • BulkUpdateOperation::__construct() and ::from()
  • ElasticDocumentAdapter::__construct()
  • ElasticEngine::__construct()

Method Signature Changes

  • BulkUpdateOperation::modelToData() - Changed from static to instance method (private, but affects anyone extending the class)

Impact: Users who manually instantiate these classes, extend them, or call the static modelToData() method will need to update their code.

Suggested Approach
Default Recommendation: Make the logger parameter optional with a fallback to NullLogger
?LoggerInterface $logger = null
...
$this->logger = $logger ?? new NullLogger();

Pros: Fully backwards compatible, seamless upgrade path
Cons: Slightly less strict dependency injection

Alternatives
Major version bump - Accept as breaking change, release as v5.0.0
Deprecation path - Keep old signatures, add new factory methods, deprecate old ones for next major version
As-is - Most users rely on DI container anyway, document the breaking change in release notes

@Jeroen-G
Which approach would you prefer?

@Jeroen-G

Copy link
Copy Markdown
Owner

Make the logger parameter optional with a fallback to NullLogger

This is what I would prefer! 👏 There is only logging if someone configures it, and it won't break existing apps that manually interact with those classes (as I know some projects do). Changes to private methods are free to break BC, there's a reason they are private.

ert485 and others added 3 commits December 31, 2025 14:06
This removes breaking changes by making the LoggerInterface parameter
optional in BulkUpdateOperation, ElasticDocumentAdapter, and
ElasticEngine constructors. When no logger is provided, a NullLogger
is used as the default.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
These assertions ensure mutation testing catches when the error context
keys are removed, preventing the ArrayItem mutation from escaping.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ert485

ert485 commented Dec 31, 2025

Copy link
Copy Markdown
Contributor Author

Ready for review @Jeroen-G

@ert485

ert485 commented Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

Is anything holding this up @Jeroen-G?

@Jeroen-G

Copy link
Copy Markdown
Owner

My motivation 🙈

@Jeroen-G
Jeroen-G merged commit 81dcfd3 into Jeroen-G:master Mar 29, 2026
@Jeroen-G

Copy link
Copy Markdown
Owner

Thanks a lot for your time, work and certainly patience!

If you could give this a quick test with requiring dev-master that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants