Skip to content

[TASK] Add trait for getArrayRepresentation to populate class - #1451

Merged
oliverklee merged 2 commits into
mainfrom
task/get-array-rep-class
Jan 2, 2026
Merged

[TASK] Add trait for getArrayRepresentation to populate class#1451
oliverklee merged 2 commits into
mainfrom
task/get-array-rep-class

Conversation

@JakeQZ

@JakeQZ JakeQZ commented Jan 1, 2026

Copy link
Copy Markdown
Collaborator

Part of #1440.

@JakeQZ
JakeQZ requested a review from oliverklee January 1, 2026 22:52
@JakeQZ JakeQZ self-assigned this Jan 1, 2026
@JakeQZ JakeQZ added testing PRs/issues adding additional tests only, or primarily testing-focused developer-specific Issues that only affect maintainers, contributors, and people submitting PRs labels Jan 1, 2026
@JakeQZ
JakeQZ force-pushed the task/get-array-rep-class branch from 29ada4a to 8f8f152 Compare January 1, 2026 23:02
@coveralls

coveralls commented Jan 1, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 70.0% (+0.03%) from 69.969%
when pulling 2a5f36e on task/get-array-rep-class
into 6604f35 on main.

@JakeQZ
JakeQZ force-pushed the task/get-array-rep-class branch from 8f8f152 to 5f41933 Compare January 1, 2026 23:05
@JakeQZ
JakeQZ force-pushed the task/get-array-rep-class branch from 5f41933 to 0f02a73 Compare January 1, 2026 23:19
@oliverklee

Copy link
Copy Markdown
Collaborator

Sorry, I missed that this is the PR to review and that #1452 is a draft PR. I'll copy my comments over.

*
* @internal
*/
public function getArrayRepresentation(): array

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.

I'd strongly prefer the trait to not have an implementation of getArrayRepresentation as this can lead to classes with an incomplete implementation if they implement the trait. (Also, I'd like to avoid parent:: calls.)

Instead, I suggest having a private method getShortClassName(): non-empty-string in the trait. Then we can also build the array from scratch in getArrayRepresentation in the implementing classes, allowing for more concise code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Instead, I suggest having a private method getShortClassName(): non-empty-string in the trait.

Changed as suggested. Also renamed the trait accordingly.

(Also, I'd like to avoid parent:: calls.)
Then we can also build the array from scratch in getArrayRepresentation in the implementing classes, allowing for more concise code.

In cases where a subclass includes additional properties and the parent also has some properties, it would be more concise not to duplicate filling in the parent's properties in the subclass's implementation.

Comment thread src/ArrayRepresentationClassProvider.php Outdated
@JakeQZ
JakeQZ requested a review from oliverklee January 2, 2026 19:20
@oliverklee
oliverklee merged commit ecde963 into main Jan 2, 2026
23 checks passed
@oliverklee
oliverklee deleted the task/get-array-rep-class branch January 2, 2026 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer-specific Issues that only affect maintainers, contributors, and people submitting PRs testing PRs/issues adding additional tests only, or primarily testing-focused

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants