Deprecate and replace findResource() method#3727
Conversation
| protected function fetchSingleRecord($id) | ||
| { | ||
| $resource = $this->table->findResource($id, 'Summon'); | ||
| $resource = $this->resourceService->getResourceByRecordId($id, 'Summon'); |
There was a problem hiding this comment.
It was actually a bug that this was previously creating resource rows when looking up IDs.
| * | ||
| * @return void | ||
| */ | ||
| public function saveRating(RecordDriver $driver, int $userId, ?int $rating): void |
There was a problem hiding this comment.
I found it confusing to have addOrUpdateRating methods at multiple layers of the code, so I gave this a different name to help differentiate it.
| * | ||
| * @return array | ||
| * | ||
| * @deprecated Use \VuFind\Ratings\RatingsService::getRatingData() |
There was a problem hiding this comment.
We can't redirect these methods without adding new record driver dependencies, so I'm just deprecating them as-is. I'd really love to get rid of them and their associated property, but we'll just have to wait until release 11.
| */ | ||
| public function __invoke() | ||
| { | ||
| return $this->service; |
There was a problem hiding this comment.
Creating a wrapper helper to expose the ratings service to the views is probably not a great design, but this falls in the category of "I don't want to change too many things all at once." I at least think it's better to have an explicit helper doing this work rather than running business logic through the record drivers, so it's a step in the right direction.
| { | ||
| $resource = $this->resourcePopulator->getOrCreateResourceForDriver($driver); | ||
| foreach ($tags as $tag) { | ||
| $resource->addTag($tag, $user); |
There was a problem hiding this comment.
This (and the deleteTag method below) will need to be refactored into \VuFind\Db\Service\TagService... but that's a project for a separate PR!
demiankatz
added
the
architecture
|
All tests are passing in both sandal and sandal5 themes. I believe this is ready for review! |
EreMaijala
left a comment
EreMaijala
left a comment
There was a problem hiding this comment.
Looks good to me and tests are passing!
2 participants
The Resource table's findResource() method does far too many different things, resulting in confusing code. This PR replaces it with three different methods covering its primary use cases, clarifying the code and allowing better organization.
Accomplishing this required some significant related refactoring: