Skip to content

Conversation

@radimvaculik
Copy link
Member

@radimvaculik radimvaculik commented Oct 30, 2025

Summary by CodeRabbit

  • Refactor
    • Introduced an abstract data model base to unify model behavior and event callbacks; tightened data type expectations for stronger type safety
  • New Features
    • Added a REST-backed data model for API-style data sources and pagination hooks
  • Breaking Change
    • Data source retrieval now expects concrete arrays instead of general iterables (may require adapter updates)

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Walkthrough

Adds an AbstractDataModel base class with event callback arrays and abstract filter methods; DataModel now extends it and no longer exposes those properties. Introduces RestDataModel implementation. Datagrid and aggregation checks updated to depend on AbstractDataModel. IDataSource::getData() return type changed from iterable to array. phpstan.neon ignore rules updated.

Changes

Cohort / File(s) Summary
New abstraction
src/AbstractDataModel.php
New abstract class defining getDataSource(): IDataSource, filterData(?DatagridPaginator, Sorting, array): iterable, filterRow(array): array, and public callback arrays onBeforeFilter, onAfterFilter, onAfterPaginated.
Data model implementations
src/DataModel.php, src/RestDataModel.php
DataModel now extends AbstractDataModel and removes its own event properties. RestDataModel added; implements datasource access, filterData lifecycle (onBeforeFilter → apply filters/sort → onAfterFilter → optional pagination → onAfterPaginated) and filterRow.
Datagrid & aggregation
src/Datagrid.php, src/AggregationFunction/TDatagridAggregationFunction.php
Type checks and property types updated from DataModel to AbstractDataModel. Datagrid adds setDataModel(AbstractDataModel): self, wires event handlers, and setDataSource() delegates through the model. Aggregation function instanceof checks updated to AbstractDataModel.
Datasource contract
src/DataSource/IDataSource.php
getData() return type changed from iterable to array.
Static analysis
phpstan.neon
Updated ignoreErrors messages to reference AbstractDataModel instead of DataModel and removed several ignore entries/comments.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Datagrid
    participant DataModel as AbstractDataModel
    participant IDataSource

    Client->>Datagrid: setDataSource(source)
    Datagrid->>Datagrid: create DataModel / RestDataModel
    Datagrid->>Datagrid: setDataModel(AbstractDataModel)
    Datagrid->>DataModel: wire onBeforeFilter/onAfterFilter/onAfterPaginated

    Client->>Datagrid: render()
    Datagrid->>DataModel: filterData(paginator?, sorting, filters)
    DataModel->>DataModel: emit onBeforeFilter[]
    DataModel->>IDataSource: getData() / apply filters & sorting
    DataModel->>DataModel: emit onAfterFilter[]
    alt paginator provided
      DataModel->>IDataSource: apply pagination (limit/offset)
      DataModel->>DataModel: emit onAfterPaginated[]
    end
    DataModel-->>Datagrid: return filtered data (array/iterable)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas needing extra attention:
    • src/AbstractDataModel.php — confirm abstract signatures and callback semantics
    • src/RestDataModel.php — verify pagination/sorting and that onAfterPaginated updates counts correctly
    • src/DataSource/IDataSource.php — ensure all implementers and consumers handle array return type
    • src/Datagrid.php — validate event wiring in setDataModel() and setDataSource() delegation

Poem

🐇 I hopped in code, a curious snout,
Built an Abstract burrow to sort things out.
DataModel stretched, RestDataModel sang,
Events were wired—hoppity clang!
Now arrays march tidy, and filters sprout. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Introducing abstract for DataModel" accurately describes the primary change in the changeset. The main objective is the creation of a new AbstractDataModel abstract base class that serves as a parent for DataModel and RestDataModel, with the extraction of common functionality including callback arrays and abstract method contracts. The title is concise, clear, and specific enough that a developer scanning the repository history would understand that this change introduces abstraction layers for the data model. The title is not vague, misleading, or off-topic—it directly addresses the most significant structural change in the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/datamodel

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dac043d and a7ae51f.

📒 Files selected for processing (7)
  • phpstan.neon (1 hunks)
  • src/AbstractDataModel.php (1 hunks)
  • src/AggregationFunction/TDatagridAggregationFunction.php (2 hunks)
  • src/DataModel.php (2 hunks)
  • src/DataSource/IDataSource.php (1 hunks)
  • src/Datagrid.php (4 hunks)
  • src/RestDataModel.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • phpstan.neon
🧰 Additional context used
🧬 Code graph analysis (6)
src/RestDataModel.php (5)
src/Components/DatagridPaginator/DatagridPaginator.php (1)
  • DatagridPaginator (22-119)
src/Utils/Sorting.php (1)
  • Sorting (5-32)
src/AbstractDataModel.php (4)
  • AbstractDataModel (14-32)
  • getDataSource (26-26)
  • filterData (28-28)
  • filterRow (30-30)
src/DataModel.php (4)
  • __construct (39-83)
  • getDataSource (85-88)
  • filterData (90-120)
  • filterRow (122-128)
src/DataSource/IDataSource.php (6)
  • filter (26-26)
  • sort (49-49)
  • limit (42-42)
  • getData (19-19)
  • getCount (14-14)
  • filterOne (33-33)
src/DataSource/IDataSource.php (8)
src/DataSource/DoctrineDataSource.php (1)
  • getData (89-102)
src/DataSource/ArrayDataSource.php (1)
  • getData (41-44)
src/DataSource/DibiFluentDataSource.php (1)
  • getData (37-40)
src/DataSource/NetteDatabaseTableDataSource.php (1)
  • getData (59-64)
src/DataSource/NextrasDataSource.php (1)
  • getData (39-47)
src/DataSource/ElasticsearchDataSource.php (1)
  • getData (54-64)
src/DataSource/DoctrineCollectionDataSource.php (1)
  • getData (48-51)
src/DataSource/ApiDataSource.php (1)
  • getData (37-47)
src/Datagrid.php (2)
src/AbstractDataModel.php (1)
  • AbstractDataModel (14-32)
src/DataModel.php (1)
  • DataModel (32-130)
src/DataModel.php (2)
src/AbstractDataModel.php (2)
  • AbstractDataModel (14-32)
  • filterRow (30-30)
src/RestDataModel.php (1)
  • filterRow (54-60)
src/AggregationFunction/TDatagridAggregationFunction.php (2)
src/Datagrid.php (1)
  • Datagrid (79-2969)
src/AbstractDataModel.php (1)
  • AbstractDataModel (14-32)
src/AbstractDataModel.php (4)
src/Components/DatagridPaginator/DatagridPaginator.php (1)
  • DatagridPaginator (22-119)
src/Utils/Sorting.php (1)
  • Sorting (5-32)
src/DataModel.php (3)
  • getDataSource (85-88)
  • filterData (90-120)
  • filterRow (122-128)
src/RestDataModel.php (3)
  • getDataSource (19-22)
  • filterData (24-52)
  • filterRow (54-60)
🔇 Additional comments (11)
src/AggregationFunction/TDatagridAggregationFunction.php (1)

5-5: LGTM! Type updated to use the new base class.

The import and instanceof check have been correctly updated to use AbstractDataModel instead of DataModel, aligning with the new abstraction layer introduced in this PR.

Also applies to: 30-30

src/DataSource/IDataSource.php (1)

19-19: LGTM! Return type correctly narrowed from iterable to array.

All concrete implementations (DoctrineDataSource, ArrayDataSource, DibiFluentDataSource, etc.) already return arrays, so this change accurately reflects the actual contract and improves type safety.

src/RestDataModel.php (3)

14-22: LGTM! Constructor and getter correctly implemented.

The constructor properly initializes the data source, and getDataSource() returns the expected type.


24-30: LGTM! Filter and non-paginated path correctly implemented.

The filter hooks and non-paginated data retrieval logic correctly follow the expected pattern.

Also applies to: 51-52


54-60: LGTM! Single row filtering correctly implemented.

The filterRow() method properly invokes lifecycle hooks and returns the filtered row data as an array.

src/Datagrid.php (3)

186-186: LGTM! Property type updated to use the abstract base class.

The dataModel property type has been correctly changed to AbstractDataModel, allowing the Datagrid to work with any data model implementation that extends the base class.


347-347: LGTM! instanceof checks correctly updated.

Both render() and setPrimaryKey() now check against AbstractDataModel instead of the concrete DataModel class, aligning with the new abstraction layer.

Also applies to: 472-472


486-502: LGTM! Data model setup correctly refactored.

The introduction of setDataModel() provides a clean extension point for custom data model implementations. The method properly wires up the three lifecycle event handlers (onBeforeFilter, onAfterFilter, onAfterPaginated), and setDataSource() now delegates to it for consistency.

src/DataModel.php (2)

32-32: LGTM! DataModel correctly extends AbstractDataModel.

The inheritance properly aligns DataModel with the new abstraction layer, allowing it to inherit the lifecycle callback arrays and implement the abstract contract.


122-128: LGTM! filterRow() return type correctly updated.

The return type has been changed from mixed to array to match the abstract method signature in AbstractDataModel. The implementation correctly delegates to filterOne()->getData() which returns an array.

src/AbstractDataModel.php (1)

1-32: LGTM! Abstract base class correctly defines the data model contract.

The AbstractDataModel class properly establishes:

  • Public lifecycle callback arrays (onBeforeFilter, onAfterFilter, onAfterPaginated) for extensibility
  • Abstract methods defining the contract for data source access, filtering, and single-row retrieval
  • Correct type hints throughout (including DatagridPaginator matching the import on line 5)

This provides a clean foundation for both DataModel and RestDataModel implementations.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1f8090 and dac043d.

📒 Files selected for processing (7)
  • phpstan.neon (1 hunks)
  • src/AbstractDataModel.php (1 hunks)
  • src/AggregationFunction/TDatagridAggregationFunction.php (2 hunks)
  • src/DataModel.php (2 hunks)
  • src/DataSource/IDataSource.php (1 hunks)
  • src/Datagrid.php (4 hunks)
  • src/RestDataModel.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/AbstractDataModel.php (5)
src/Datagrid.php (2)
  • Datagrid (79-2969)
  • getDataSource (504-509)
src/Components/DatagridPaginator/DatagridPaginator.php (1)
  • DatagridPaginator (22-119)
src/Utils/Sorting.php (1)
  • Sorting (5-32)
src/DataModel.php (3)
  • getDataSource (85-88)
  • filterData (90-120)
  • filterRow (122-128)
src/RestDataModel.php (3)
  • getDataSource (19-22)
  • filterData (24-52)
  • filterRow (54-60)
src/DataSource/IDataSource.php (8)
src/DataSource/DoctrineDataSource.php (1)
  • getData (89-102)
src/DataSource/DibiFluentDataSource.php (1)
  • getData (37-40)
src/DataSource/ArrayDataSource.php (1)
  • getData (41-44)
src/DataSource/NetteDatabaseTableDataSource.php (1)
  • getData (59-64)
src/DataSource/NextrasDataSource.php (1)
  • getData (39-47)
src/DataSource/ElasticsearchDataSource.php (1)
  • getData (54-64)
src/DataSource/DoctrineCollectionDataSource.php (1)
  • getData (48-51)
src/DataSource/ApiDataSource.php (1)
  • getData (37-47)
src/Datagrid.php (2)
src/AbstractDataModel.php (1)
  • AbstractDataModel (14-32)
src/DataModel.php (1)
  • DataModel (32-130)
src/AggregationFunction/TDatagridAggregationFunction.php (1)
src/AbstractDataModel.php (1)
  • AbstractDataModel (14-32)
src/DataModel.php (2)
src/AbstractDataModel.php (2)
  • AbstractDataModel (14-32)
  • filterRow (30-30)
src/RestDataModel.php (1)
  • filterRow (54-60)
src/RestDataModel.php (5)
src/Components/DatagridPaginator/DatagridPaginator.php (1)
  • DatagridPaginator (22-119)
src/Utils/Sorting.php (1)
  • Sorting (5-32)
src/AbstractDataModel.php (4)
  • AbstractDataModel (14-32)
  • getDataSource (26-26)
  • filterData (28-28)
  • filterRow (30-30)
src/DataModel.php (4)
  • __construct (39-83)
  • getDataSource (85-88)
  • filterData (90-120)
  • filterRow (122-128)
src/DataSource/IDataSource.php (6)
  • filter (26-26)
  • sort (49-49)
  • limit (42-42)
  • getData (19-19)
  • getCount (14-14)
  • filterOne (33-33)
🔇 Additional comments (11)
src/DataModel.php (2)

32-32: LGTM: DataModel now properly extends AbstractDataModel.

The inheritance hierarchy is well-structured, moving event callbacks and abstract method contracts to the base class.


122-128: LGTM: Return type correctly updated to array.

The return type change from mixed to array aligns with the AbstractDataModel contract and the updated IDataSource.getData() signature.

phpstan.neon (1)

33-39: LGTM: PHPStan ignoreErrors correctly updated to AbstractDataModel.

The error suppressions now reference the new abstract base class, aligning with the type changes in Datagrid.

src/AggregationFunction/TDatagridAggregationFunction.php (1)

5-5: LGTM: Type constraint correctly updated to AbstractDataModel.

The instanceof check now accepts any AbstractDataModel implementation (DataModel or RestDataModel), properly supporting the new abstraction.

Also applies to: 30-32

src/RestDataModel.php (2)

14-22: LGTM: Clean constructor accepting IDataSource directly.

RestDataModel provides a simpler alternative to DataModel by accepting an IDataSource directly, whereas DataModel wraps various data source types. This is a good separation of concerns.


54-60: LGTM: filterRow implementation is correct.

The single-row filtering logic properly fires lifecycle hooks and returns the filtered data as an array.

src/AbstractDataModel.php (1)

17-24: LGTM: Event callback arrays follow Nette patterns.

The public event callback arrays with PHPDoc magic method annotations follow standard Nette framework conventions for event handling.

src/Datagrid.php (3)

186-186: LGTM: Property type correctly updated to AbstractDataModel.

The type change enables Datagrid to work with both DataModel and RestDataModel implementations, properly supporting the new abstraction.


347-349: LGTM: Validation correctly checks for AbstractDataModel.

The instanceof check appropriately validates that a data model (of any implementation) has been set before rendering.


486-502: LGTM: Well-structured refactoring with proper event wiring.

The refactoring properly separates concerns:

  • setDataSource() wraps various source types in DataModel and delegates to setDataModel()
  • setDataModel() accepts any AbstractDataModel and wires lifecycle event callbacks

The event wiring connects the data model's hooks to the aggregation function handlers from TDatagridAggregationFunction trait.

src/DataSource/IDataSource.php (1)

19-19: All IDataSource implementations already return array type — interface change is compatible.

Verification confirms that all 9 implementations (DoctrineDataSource, ArrayDataSource, NetteDatabaseTableDataSource, NetteDatabaseTableMssqlDataSource, ApiDataSource, NextrasDataSource, ElasticsearchDataSource, DibiFluentDataSource, and DoctrineCollectionDataSource) already return array from getData(). The change from iterable to array in the interface is a safe narrowing of the contract and compatible with all existing implementations.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants