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

Convert the non-entity Service classes to use Laravel's ServiceContainer #7131

Open
NateWr opened this issue Jun 15, 2021 · 8 comments
Open
Assignees
Labels
Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day.
Milestone

Comments

@NateWr
Copy link
Contributor

NateWr commented Jun 15, 2021

We have a few service classes that do not control a specific entity, like a submission, publication or user. Instead they are helper services, like our schema service for working with entity schemas and our stats services for getting usage and editorial stats.

These should be refactored to use Laravel's ServiceContainer, so that they can take advantage of automatic dependency injection. We can then remove our dependency on the pimple service container and our Services::get(...) facade.

The following service classes need to be refactored:

  • FileService
  • SchemaService
  • StatsService
  • StatsEditorialService
  • NavigationMenuService (this one may be better split up into 2 entity services)
@NateWr NateWr added the Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. label Jun 15, 2021
@NateWr NateWr added this to the OJS/OMP/OPS 3.4 milestone Jun 15, 2021
@NateWr NateWr self-assigned this Jun 15, 2021
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 15, 2021
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 16, 2021
NateWr added a commit to NateWr/lensGalley that referenced this issue Jun 16, 2021
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 16, 2021
@NateWr
Copy link
Contributor Author

NateWr commented Jun 16, 2021

The following PRs refactor the FileService to use Laravel's Service Container instead of the Services facade. This makes it available for automatic dependency injection. With this approach, it is also possible for a plugin to modify the file storage adaptor (eg - to use AWS or something) by swapping out the FileService with their own.

PRs:
#7137
pkp/ojs#3155
asmecher/lensGalley#49
pkp/omp#987
pkp/ops#165

NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 16, 2021
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 16, 2021
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 28, 2021
@Vitaliy-1
Copy link
Collaborator

What about registering FileService as Laravel FileServiceProvider to adopt its interface?

NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 28, 2021
@NateWr
Copy link
Contributor Author

NateWr commented Jun 29, 2021

Do you have an example, @Vitaliy-1? From what I've gathered by looking at the docs, service providers are essentially classes that register functionality. But not necessarily classes that perform the functionality themselves. In other words, they bootstrap services but are not services themselves.

So in this case I've used an AppServiceProvider that registers things like the file service: https://github.com/pkp/pkp-lib/pull/7137/files#diff-79308d3915f34d09bc92323605cc26b94554a5b5efd966098d7be21dffc7a47dR39. Am I misunderstanding how service providers work?

@Vitaliy-1
Copy link
Collaborator

I meant Laravel filesystem bindings, that are originally registered through FilesystemServiceProvider. As far as I remember these bindings are used in CacheServiceProvider and several other services that we may want to adopt in the future.
This would require also replacing/adapting PKPFileService with Laravel's Filesystem, as I understand Laravel uses it on top of the Flysystem

@Vitaliy-1
Copy link
Collaborator

Looking at Laravel's Filesystem, its methods have quite different implementations from what we use, thus it may only complicate things.

@asmecher
Copy link
Member

asmecher commented Feb 9, 2023

@Vitaliy-1 and @NateWr, any objections to bumping this wholesale to post-3.4.0? Or are there parts that should be salvaged for 3.4?

@NateWr NateWr modified the milestones: 3.4, 3.4.x Feb 9, 2023
@NateWr
Copy link
Contributor Author

NateWr commented Feb 9, 2023

👍 no objections

@Vitaliy-1
Copy link
Collaborator

As part of the work on localizations (see the commit) Laravel's FilesystemServiceProvider was registered within our container as it's required for the Illuminate\Cache\CacheManager to initialize Illuminate\Cache\FileStore to store the cache by the specified in the config path, see: https://github.com/laravel/framework/blob/16454f17a2585c4589f721655fc5133270aadf8c/src/Illuminate/Cache/CacheManager.php#L147

It's registered together with the Fly's Filesystem and the part of it's initialization in PKPFileService could be moved there. We don't need to create a separate Service for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day.
Projects
None yet
Development

No branches or pull requests

4 participants