pkp-lib icon indicating copy to clipboard operation
pkp-lib copied to clipboard

Replace pimple with laravel's dependency injection container

Open touhidurabir opened this issue 1 year ago • 3 comments

Describe the proposal There are some implementations in the codebase that use the Pimple for the purpose of dependency injection. This package is a part of micro framework Silex which as been deprecated quite some times ago. Also as we have laravel's own dependency injection container, it's better to use that as we are adopting more from laravel's toolset .

What application are you using? OJS, OMP or OPS version main (3.5.0 pre release)

Additional information Only few classes are using it such as

  1. PKP\core\PKPServices
  2. APP\core\Services
  3. APP\services\OJSServiceProvider
  4. APP\services\OMPServiceProvider
  5. APP\services\OPSServiceProvider

Probably a good approach is to utilize the app or shared lib(pkp-lib) specific AppServiceProvider to put the functionalities there .

PRs pkp-lib --> https://github.com/pkp/pkp-lib/pull/10177 ojs --> https://github.com/pkp/ojs/pull/4358 omp --> https://github.com/pkp/omp/pull/1633 ops --> https://github.com/pkp/ops/pull/728 crossref-ojs --> https://github.com/pkp/crossref-ojs/pull/51 jatsTemplate --> https://github.com/pkp/jatsTemplate/pull/45 oaiJats --> https://github.com/pkp/oaiJats/pull/45 plagiarism --> https://github.com/pkp/plagiarism/pull/61 crossref-ops --> https://github.com/pkp/crossref-ops/pull/41

touhidurabir avatar Apr 25 '24 11:04 touhidurabir

I think this can be closed as a duplicate of https://github.com/pkp/pkp-lib/issues/7131

jonasraoni avatar Apr 25 '24 13:04 jonasraoni

@asmecher can you check the PRs at https://github.com/pkp/pkp-lib/issues/9913#issue-2263387287. This simple PRs centralize all services to use the laravel container and remove pimple container . If all ok, I plan to remove all reference of Services::get with app()->get() and port it to OMP and OPS .

touhidurabir avatar Jul 10 '24 05:07 touhidurabir

Looks good, @touhidurabir, please go ahead! Also document any code changes that might be necessary for 3rd parties in the top comment, and add a link to it in https://github.com/pkp/pkp-lib/issues/9276.

asmecher avatar Jul 11 '24 01:07 asmecher

@touhidurabir, thanks, I've reviewed the PRs here and they look good. Could you...

  • Rebase the PRs as necessary to resolve conflicts,
  • Ensure that tests are passing,
  • Ensure that all plugin repos have distinct stable-3_4_0 branches, and finally
  • Merge the PRs into main?

asmecher avatar Jul 30 '24 23:07 asmecher

@asmecher

  1. rebased
  2. all tests are passing for OJS, OMP and OPS .
  3. Only plagiarism had missing stable-3_4_0 branch and I have created it from main .

it's ready for merge but I do not have write access to few plugins repo . Can you please merge it as it's better to have all merged at same time to avoid conflict .

touhidurabir avatar Aug 01 '24 07:08 touhidurabir

All merged, thanks! Closing.

asmecher avatar Aug 01 '24 15:08 asmecher