Register Laravel service providers to allow use of global functions

pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.

https://pkp.sfu.ca

GNU General Public License v3.0

299 stars 444 forks source link

Register Laravel service providers to allow use of global functions #6915

Closed NateWr closed 3 years ago

NateWr commented 3 years ago

The Laravel service providers we have integrated so far are decoupled in a way that prevents some of Laravel's facades and global functions from being used. These can be registered together in a container to support these global functions as well as a cleaner integration of these providers in pkp-lib's bootstrap.

Vitaliy-1 commented 3 years ago

PRs: usageStats main: https://github.com/pkp/usageStats/pull/47 pkp-lib main: https://github.com/pkp/pkp-lib/pull/6939 staticPages main: https://github.com/pkp/staticPages/pull/66 OJS main: https://github.com/pkp/ojs/pull/3103

Vitaliy-1 commented 3 years ago

@NateWr, regarding our conversation:

Validation Service Provider requires also registration of File Service and Laravel Localizations. This is a fair amount of work, I mean wrap our File Management and localizations into these service provider, so I haven't started the work on it yet.
There is a place with a code duplication in PKPEmailHandler, in particular: https://github.com/pkp/pkp-lib/blob/1cbb1890e1f79817dfda3071a92479e06484d787/api/v1/_email/PKPEmailHandler.inc.php#L204-L211. QueueServiceProvider had already registered the worker before the handler initialization. As far as I understand, Laravel starts worker through the Job, thus, to mimic this behavior, I think, we should create a Job class: https://laravel.com/docs/7.x/queues#class-structure and dispatch it through our handler, like: https://laravel.com/docs/7.x/queues#dispatching-jobs. I haven't done anything regarding applying this architecture to our codebase.

NateWr commented 3 years ago

Validation Service Provider requires also registration of File Service and Laravel Localizations

Ok, let's ignore this for now. We can revisit this later.

There is a place with a code duplication in PKPEmailHandler

Let's leave this alone for now and address it as part of #4622. The PKPEmailHandler approach was a temporary solution for the Notify function in 3.3, but the plan is to refactor it once we have better queue processing infrastructure.

NateWr commented 3 years ago

This looks great, @Vitaliy-1. @asmecher do you mind taking a look at this? This work adopts Laravel's centralized service provider registry which allows us to make direct use of Laravel's facades. This means we can adopt some of Laravel's things more directly, eg:

use Illuminate\Support\Facades\DB;

DB::table(...);

It also means that the components of Laravel that we adopt can "talk to each other", which is important for using Laravel features like creating an Event with a ShouldQueue trait to automatically send it to the queue.

The key changes are in PKPApplication at https://github.com/pkp/pkp-lib/pull/6939/files#diff-1f9b4a313ad0dfc341b0ed9d6d38cdf0588e4c533f9a3d346198648fe172bff0 and PKPContainer at https://github.com/pkp/pkp-lib/pull/6939/files#diff-aea06926e59aebf7d362a9e19ed70115727b562536ecd5a039c326f2068de007.

If possible, I'd like to merge this in this week before the formatting change at #5678.

Vitaliy-1 commented 3 years ago

Finally, test builds are successful. What I forgot is to add licensing info and description to the newly created classes.

asmecher commented 3 years ago

This looks very good, @Vitaliy-1 -- aside from renaming the classes used to access Laravel features it's actually just a simplification/encapsulation of the initialization code. [And extension of its capabilities!] I added a comment or two just checking on the queuing features -- have you checked those to see if they work after the removal/refactoring of some of the related bootstrapping?

If the batch email send works OK with a couple "pages" of addresses, I'm OK to get this merged anytime!

Vitaliy-1 commented 3 years ago

Forgot to check OMP and OPS

Vitaliy-1 commented 3 years ago

OMP: https://github.com/pkp/omp/pull/958 OPS: https://github.com/pkp/ops/pull/144

Vitaliy-1 commented 3 years ago

(Waiting for builds to finish before merging)

Vitaliy-1 commented 3 years ago

All OPS builds are failing when trying to find

cy.login('dbarnes');
cy.visit('index.php/publicknowledge/workflow/access/1');
cy.get('.ui-tabs-anchor').contains('Submission').

I don't think we have the Submission tab in the workflow of OPS, don't we? test

NateWr commented 3 years ago

No, only production

On Thu, 15 Apr 2021, 18:12 Vitaliy, @.***> wrote:

All OPS builds are failing when trying to find

cy.login('dbarnes'); cy.visit('index.php/publicknowledge/workflow/access/1'); cy.get('.ui-tabs-anchor').contains('Submission').

I don't think we have the Submission tab in the workflow of OPS, don't we? [image: test] https://user-images.githubusercontent.com/20665587/114910359-7a231f00-9e26-11eb-8f87-ba9c665067cf.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pkp/pkp-lib/issues/6915#issuecomment-820594000, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARTERIY7HQD3KIXJ3Q5UXLTI4M67ANCNFSM42OLJMBQ .