Skip to content

Conversation

Tigrov
Copy link
Member

@Tigrov Tigrov commented Jun 10, 2025

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues #363

Copy link

codecov bot commented Jun 10, 2025

Codecov Report

❌ Patch coverage is 72.41379% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.94%. Comparing base (10cd947) to head (3aadcdc).

Files with missing lines Patch % Lines
src/Trait/EventsTrait.php 72.13% 17 Missing ⚠️
src/Event/Handler/SetValueOnUpdate.php 44.82% 16 Missing ⚠️
src/Event/Handler/DefaultValue.php 0.00% 12 Missing ⚠️
src/Event/Handler/DefaultValueOnInsert.php 59.09% 9 Missing ⚠️
src/Event/AbstractEvent.php 85.71% 2 Missing ⚠️
src/Event/AfterDelete.php 0.00% 2 Missing ⚠️
src/Event/AfterUpsert.php 0.00% 2 Missing ⚠️
src/Event/BeforeUpsert.php 0.00% 2 Missing ⚠️
src/Event/EventDispatcherProvider.php 92.59% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #428      +/-   ##
============================================
- Coverage     89.16%   86.94%   -2.23%     
- Complexity      572      666      +94     
============================================
  Files            18       40      +22     
  Lines          1385     1616     +231     
============================================
+ Hits           1235     1405     +170     
- Misses          150      211      +61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@samdark samdark requested a review from Copilot June 11, 2025 06:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a new event-dispatching system to ActiveRecord via attributes, letting models attach handlers for common events such as insert, update, and populate.

  • Introduces EventDispatcher, EventInterface, various event classes (e.g. BeforeInsert, AfterUpdate), and handler interfaces/abstract classes
  • Adds handler attributes (DefaultValueOnInsert, DefaultValue, SetDateTimeOnUpdate, DefaultDateTimeOnInsert) to set default values or timestamps
  • Updates composer.json to require yiisoft/event-dispatcher

Reviewed Changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Event/EventDispatcher.php Implements event discovery via attributes and listener registration
src/Event/Handler/HandlerInterface.php Defines the interface for attribute-based event handlers
src/Event/Handler/AbstractHandler.php Base class managing property names and default event list
src/Event/Handler/DefaultValueOnInsert.php Handler that sets default values on BeforeInsert
src/Event/Handler/DefaultValue.php Handler that sets default values on AfterPopulate
src/Event/Handler/DefaultDateTimeOnInsert.php Inserts timestamps on BeforeInsert
src/Event/Handler/SetDateTimeOnUpdate.php Updates timestamps on BeforeUpdate
src/Event/*.php New event classes covering save, insert, update, delete, populate
composer.json Adds yiisoft/event-dispatcher to requirements and suggestions
Comments suppressed due to low confidence (2)

src/Event/Handler/HandlerInterface.php:20

  • [nitpick] Consider adding a PHPDoc comment for the handle method to explain what kinds of events it processes and any expectations about modification of the event or model.
public function handle(EventInterface $event): void;

src/Event/Handler/SetDateTimeOnUpdate.php:1

  • No unit tests were added for SetDateTimeOnUpdate; please add tests to verify default timestamp generation and custom property name handling.
<?php

# Conflicts:
#	tests/ActiveRecordTest.php
Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

I love this implementation. Let's add docs for it and missing tests. Awesome job. Waiting forward to use it.

How does it affect performance btw.?

@samdark
Copy link
Member

samdark commented Jun 15, 2025

A good test for it and also a handy functionality could be SoftDelete.

  1. deleted_at that is filled on delete(). If it's null then record is not deleted.
  2. If one is referencing a deleted record from relation, it's ignored silently.
  3. A way to get all records including deleted ones or deleted ones only.

@Tigrov Tigrov added the status:under development Someone is working on a pull request. label Jun 28, 2025
@xepozz
Copy link
Member

xepozz commented Jun 28, 2025

Please no manipulations in the constructor

Tigrov and others added 11 commits August 4, 2025 23:25
# Conflicts:
#	src/AbstractActiveRecord.php
#	src/ActiveQuery.php
# Conflicts:
#	src/AbstractActiveRecord.php
#	src/ActiveRecordInterface.php
#	src/Trait/FactoryTrait.php
# Conflicts:
#	tests/ActiveQueryTest.php
#	tests/Driver/Oracle/ActiveQueryTest.php
@samdark
Copy link
Member

samdark commented Aug 10, 2025

Looks 👍 overall. Need docs.

@Tigrov Tigrov removed the status:under development Someone is working on a pull request. label Sep 3, 2025
@vjik vjik mentioned this pull request Sep 8, 2025
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.

6 participants