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(TextProcessing): Add task type template param to IManager and registerTPProvider #41844

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

marcelklehr
Copy link
Member

@marcelklehr marcelklehr commented Nov 29, 2023

otherwise we cannot use them reasonably

Checklist

@marcelklehr
Copy link
Member Author

/backport to stable28

@marcelklehr
Copy link
Member Author

/backport to stable27

@provokateurin
Copy link
Member

Where and why is it failing?

@marcelklehr
Copy link
Member Author

marcelklehr commented Nov 29, 2023

It is failing when using implementations of IProvider, ie. when registering a provider or when passing a Task object to IManager#runTask or IManager#scheduleTask. See https://psalm.dev/docs/annotating_code/templated_annotations/#template-covariance

@AndyScherzinger AndyScherzinger added this to the Nextcloud 29 milestone Nov 29, 2023
@AndyScherzinger AndyScherzinger added the 3. to review Waiting for reviews label Nov 29, 2023
@come-nc
Copy link
Contributor

come-nc commented Nov 30, 2023

Can you give a minimal example on what did not work which will now work, ideally on psalm.dev?

@marcelklehr
Copy link
Member Author

@marcelklehr
Copy link
Member Author

and https://psalm.dev/r/62efc9b86a

@marcelklehr
Copy link
Member Author

I guess just adding a template param to the affected methods as well will fix this as well:

https://psalm.dev/r/8fb724b093
https://psalm.dev/r/6d44eb50cd

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Thanks for the details 👍

@marcelklehr
Copy link
Member Author

@come-nc Do you think we should do the covariant or add new template to the methods in questions as in my latest comment?

@marcelklehr marcelklehr force-pushed the fix/tp-api-covariance branch from a6e2d8d to 6c5164c Compare November 30, 2023 12:29
@marcelklehr marcelklehr changed the title fix(TextProcessing): Make task type template param of IProvider and T… fix(TextProcessing): Add task type template param to IManager and registerTPProvider Nov 30, 2023
lib/public/TextProcessing/IManager.php Fixed Show fixed Hide fixed
lib/public/TextProcessing/IManager.php Fixed Show fixed Hide fixed
lib/public/TextProcessing/IManager.php Fixed Show fixed Hide fixed
lib/public/TextProcessing/IManager.php Fixed Show fixed Hide fixed
@marcelklehr marcelklehr force-pushed the fix/tp-api-covariance branch from 038afc8 to 68d59d3 Compare November 30, 2023 15:11
@come-nc
Copy link
Contributor

come-nc commented Nov 30, 2023

@come-nc Do you think we should do the covariant or add new template to the methods in questions as in my latest comment?

The covariant thing, because you might have a function that accept both template<B> and template<C> if both inherit A.
Then if you have a function which only accepts template<SubType> you should also reflect it in phpdoc typing but that’s another story.

@provokateurin
Copy link
Member

@come-nc Agreed.

@marcelklehr marcelklehr force-pushed the fix/tp-api-covariance branch from fb49537 to abf8655 Compare December 1, 2023 07:55
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr marcelklehr force-pushed the fix/tp-api-covariance branch from abf8655 to fe6d9e3 Compare December 1, 2023 10:52
@marcelklehr marcelklehr merged commit e27e2e4 into master Dec 1, 2023
51 checks passed
@marcelklehr marcelklehr deleted the fix/tp-api-covariance branch December 1, 2023 12:24
@marcelklehr
Copy link
Member Author

/backport to stable27

@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants