feat(notifications): provide method to preload many notifications at once#54232
feat(notifications): provide method to preload many notifications at once#54232
Conversation
st3iny
added
enhancement
2. developing
| * improve performance by, for example, bundling SQL queries. | ||
| */ | ||
| #[Implementable(since: '32.0.0')] | ||
| interface IPreloadableNotifier extends INotifier { |
There was a problem hiding this comment.
We should deprecate INotifier (but can also wait one release), to make developers more aware of the performance related improvement
There was a problem hiding this comment.
I would rather integrate the method into the actual interface rather than deprecating it. Otherwise, Psalm will complain all the time about INotifier being deprecated and we will end up with IPreloadableNotifier becoming the base interface at some point which sounds weird.
I added a hint to INotifier though to make developers aware.
There was a problem hiding this comment.
Well we can not extend the base interface due to our policies
And deprecating the other one would help via psalm and IDEs that was exactly my idea to it.
If an app doesn't care they can implement the interface and do nothing in the method, but get easily rid of the deprecation?
There was a problem hiding this comment.
I'd vote for keeping the old interface as-is because the usage is not always problematic. With documentation (manual, code in-line) we can get attention to the extending interface for apps where preloading makes sense.
st3iny
added
3. to review
st3iny
requested review from
a team,
ChristophWurst,
SebastianKrupinski,
miaulalala and
tcitworld
as code owners
st3iny
requested review from
ArtificialOwl,
nickvergessen,
provokateurin and
yemkareems
and removed request for
a team
| * improve performance by, for example, bundling SQL queries. | ||
| */ | ||
| #[Implementable(since: '32.0.0')] | ||
| interface IPreloadableNotifier extends INotifier { |
There was a problem hiding this comment.
I'd vote for keeping the old interface as-is because the usage is not always problematic. With documentation (manual, code in-line) we can get attention to the extending interface for apps where preloading makes sense.
…once Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
st3iny
force-pushed
the
feat/notifications/preload-many
branch
from
a2fb8d9 to
ad39dab
Compare
st3iny
added
4. to release
ChristophWurst
added
the
pending documentation
|
|
4 participants
INotifier::preloadMany()to allow implementations to preload and cache data for many notifications at once #54231Summary
Allow notifier implementations to preload and cache data for many notifications at once to improve performance by, for example, bundling SQL queries.
Checklist