Skip to content

Conversation

@pavel-patek-mzk
Copy link
Contributor

Description
This PR refactors ObalkyKnihService::getFromService by extracting the query-building logic into a separate protected method.

Why
The goal is to allow overriding only the query construction without duplicating the full getFromService implementation.

Use case
In our case, we need to extend the query with additional parameters (part_no, part_year, part_volume) so that the service can fetch covers for specific periodical/issue instances.

@demiankatz demiankatz added this to the 11.0 milestone Sep 2, 2025
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @pavel-patek-mzk! The refactoring makes sense to me; it just looks like there's a minor style problem in the comments that needs to be adjusted to get the build to pass. I recommend getting in the habit of running composer fix before submitting pull requests, since this automatically corrects many common formatting problems and also will inform you of areas requiring manual attention.

Additionally, I notice that there is currently no unit test coverage for ObalkyKnihService. That's not a requirement to get this improvement merged, but it might be a nice addition to consider for the future.

@demiankatz demiankatz changed the title Refactor ObalkyKnihService extract query building into separate method Refactor ObalkyKnihService: extract query building into separate method Sep 2, 2025
@pavel-patek-mzk
Copy link
Contributor Author

Thanks for the feedback! I've applied composer fix to address the style issues. I’ll consider adding unit tests for ObalkyKnihService in a future improvement.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @pavel-patek-mzk -- tests are passing now, so I will merge this.

@demiankatz demiankatz merged commit 3730459 into vufind-org:dev Sep 2, 2025
6 checks passed
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.

2 participants