Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix up generic docblocks. #16651

Merged
merged 1 commit into from
Jul 29, 2022
Merged

Fix up generic docblocks. #16651

merged 1 commit into from
Jul 29, 2022

Conversation

dereuromark
Copy link
Member

@dereuromark dereuromark commented Jul 25, 2022

The original (legacy syntax) was:

@param \Traversable|\Cake\Datasource\EntityInterface[]

which is meaning wise identical to

@param \Traversable<\Cake\Datasource\EntityInterface>

at least in the given context, since there was no other way of stating an object collection.

Now, given the legacy syntax there was always not clear which one was meant, as it could also mean

@param \Traversable|array<\Cake\Datasource\EntityInterface

which is the current documentation - and also reflects reality (partially).

However, that is a different meaning, as other code examples show, where it is either an object, or an array.
Not a collection of either object or array with the same type inside.

Those are both collection types here (object + array with each the same key).
As such, shoudnt those be fixed up to

@param \Traversable<\Cake\Datasource\EntityInterface>|array<\Cake\Datasource\EntityInterface>

then? As both are the collection we want to iterate over in the end.

There could in the future also be more normalized versions of this, e.g.

@param (\Traversable|array)<\Cake\Datasource\EntityInterface

But for now the verbose syntax is only supported by tooling.

PS: This does need small docblock updates on the actual interface, to make it "generic", afaik.

@dereuromark
Copy link
Member Author

TooManyTemplateParams
=> isnt that easily solved by adding annotations on the interface?
see https://github.com/propelorm/Propel2/pull/1859/files#diff-b77fcf24b104df82969824d2662b771bfe9a87cc4186caa1ce2c9cc5740d84dcR22

@markstory markstory modified the milestones: 4.4.3, 4.4.4 Jul 27, 2022
@@ -2178,9 +2178,9 @@ protected function _update(EntityInterface $entity, array $data)
* any one of the records fails to save due to failed validation or database
* error.
*
* @param \Cake\Datasource\ResultSetInterface|array<\Cake\Datasource\EntityInterface> $entities Entities to save.
* @param \Cake\Datasource\ResultSetInterface<\Cake\Datasource\EntityInterface>|array<\Cake\Datasource\EntityInterface> $entities Entities to save.
Copy link
Member

Choose a reason for hiding this comment

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

The simplest solution might probably be using iterable<\Cake\Datasource\EntityInterface>. The methods related to save/delete many simply loop over the argument anyway and don't use any resultset specific feature.

@markstory markstory merged commit 44c9269 into 4.x Jul 29, 2022
@markstory markstory deleted the bugfix/annotations-generic branch July 29, 2022 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants