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

Fatal error at PKPEmailHandler on bulk email sending #8734

Closed jonasraoni closed 1 year ago

jonasraoni commented 1 year ago

Describe the bug See error log below, the origin is here: https://github.com/pkp/pkp-lib/blob/566c546eccc8be374181172412803a06695ec9dc/api/v1/_email/PKPEmailHandler.php#L168

And fails due to this bug in Laravel (you cannot call isset($closure->property) on a closure): https://github.com/laravel/framework/blob/ab7586b3b7303958f343c84531b021b1055bd6a3/src/Illuminate/Queue/Queue.php#L328

Slim Application Error:
Type: Error
Message: Closure object cannot have properties
File: /var/www/html/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Queue/Queue.php
Line: 328
Trace: #0 /var/www/html/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Queue/Queue.php(304): Illuminate\Queue\Queue->shouldDispatchAfterCommit(Object(Closure))
#1 /var/www/html/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Queue/DatabaseQueue.php(97): Illuminate\Queue\Queue->enqueueUsing(Object(Closure), '{"uuid":"9adb69...', 'email_640120019...', NULL, Object(Closure))
#2 /var/www/html/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Queue/QueueManager.php(291): Illuminate\Queue\DatabaseQueue->push(Object(Closure), Array, 'email_640120019...')
#3 /var/www/html/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php(337): Illuminate\Queue\QueueManager->__call('push', Array)
#4 /var/www/html/lib/pkp/api/v1/_email/PKPEmailHandler.php(168): Illuminate\Support\Facades\Facade::__callStatic('push', Array)
#5 [internal function]: PKP\API\v1\_email\PKPEmailHandler->create(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Array)
#6 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/Handlers/Strategies/RequestResponse.php(40): call_user_func(Array, Object(Slim\Http\Request), Object(PKP\core\APIResponse), Array)
#7 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/Route.php(281): Slim\Handlers\Strategies\RequestResponse->__invoke(Array, Object(Slim\Http\Request), Object(PKP\core\APIResponse), Array)
#8 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(117): Slim\Route->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#9 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/Route.php(268): Slim\Route->callMiddlewareStack(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#10 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/App.php(503): Slim\Route->run(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#11 /var/www/html/lib/pkp/classes/security/authorization/internal/ApiAuthorizationMiddleware.php(87): Slim\App->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#12 [internal function]: PKP\security\authorization\internal\ApiAuthorizationMiddleware->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Slim\App))
#13 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/DeferredCallable.php(57): call_user_func_array(Object(PKP\security\authorization\internal\ApiAuthorizationMiddleware), Array)
#14 [internal function]: Slim\DeferredCallable->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Slim\App))
#15 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(70): call_user_func(Object(Slim\DeferredCallable), Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Slim\App))
#16 /var/www/html/lib/pkp/classes/security/authorization/internal/ApiCsrfMiddleware.php(54): Slim\App->Slim\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#17 [internal function]: PKP\security\authorization\internal\ApiCsrfMiddleware->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#18 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/DeferredCallable.php(57): call_user_func_array(Object(PKP\security\authorization\internal\ApiCsrfMiddleware), Array)
#19 [internal function]: Slim\DeferredCallable->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#20 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(70): call_user_func(Object(Slim\DeferredCallable), Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#21 /var/www/html/lib/pkp/classes/security/authorization/internal/ApiTokenDecodingMiddleware.php(141): Slim\App->Slim\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#22 [internal function]: PKP\security\authorization\internal\ApiTokenDecodingMiddleware->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#23 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/DeferredCallable.php(57): call_user_func_array(Object(PKP\security\authorization\internal\ApiTokenDecodingMiddleware), Array)
#24 [internal function]: Slim\DeferredCallable->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#25 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(70): call_user_func(Object(Slim\DeferredCallable), Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#26 /var/www/html/lib/pkp/classes/handler/APIHandler.php(84): Slim\App->Slim\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#27 [internal function]: PKP\handler\APIHandler->PKP\handler\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#28 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/DeferredCallable.php(57): call_user_func_array(Object(Closure), Array)
#29 [internal function]: Slim\DeferredCallable->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#30 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(70): call_user_func(Object(Slim\DeferredCallable), Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#31 /var/www/html/lib/pkp/classes/handler/APIHandler.php(143): Slim\App->Slim\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#32 [internal function]: PKP\handler\APIHandler->PKP\handler\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#33 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/DeferredCallable.php(57): call_user_func_array(Object(Closure), Array)
#34 [internal function]: Slim\DeferredCallable->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#35 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(70): call_user_func(Object(Slim\DeferredCallable), Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#36 /var/www/html/lib/pkp/classes/handler/APIHandler.php(148): Slim\App->Slim\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#37 [internal function]: PKP\handler\APIHandler->PKP\handler\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#38 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/DeferredCallable.php(57): call_user_func_array(Object(Closure), Array)
#39 [internal function]: Slim\DeferredCallable->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#40 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(70): call_user_func(Object(Slim\DeferredCallable), Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#41 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(117): Slim\App->Slim\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#42 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/App.php(392): Slim\App->callMiddlewareStack(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#43 /var/www/html/lib/pkp/classes/handler/APIHandler.php(140): Slim\App->process(Object(Slim\Http\Request), Object(PKP\core\APIResponse))
#44 [internal function]: PKP\handler\APIHandler->PKP\handler\{closure}(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#45 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/DeferredCallable.php(57): call_user_func_array(Object(Closure), Array)
#46 [internal function]: Slim\DeferredCallable->__invoke(Object(Slim\Http\Request), Object(PKP\core\APIResponse), Object(Closure))
#47 /var/www/html/lib/pkp/lib/vendor/slim/slim/Slim/MiddlewareAwareTrait.php(70): call_user_func(Object(Slim\DeferredCallable), Object(Slim\Http\Request), Object(PKP\core\

Re-production Steps

  1. Make sure that bulk email feature is enable for context(journal/server/...) . Enable it from Administration --> Site Settings --> Bulk Emails --> Select specific context --> Save
  2. Go to Context(Journal/Server/....) --> User & Roles --> Notify
  3. Try to send bulk mail

What application are you using? OJS 3.4.0 RC1

Additional Information Details reason at https://github.com/pkp/pkp-lib/issues/8734#issuecomment-1471632392

PRs pkp-lib --> https://github.com/pkp/pkp-lib/pull/8827 ui-library --> https://github.com/pkp/ui-library/pull/270 ojs --> https://github.com/pkp/ojs/pull/3838 [TEST ONLY]

jonasraoni commented 1 year ago

I've created an issue at the Laravel repository https://github.com/laravel/framework/issues/46355

asmecher commented 1 year ago

@jonasraoni, what are the reproduction steps to cause this? (This will help determine whether it's a must-fix for 3.4 or can wait on Laravel's schedule.)

touhidurabir commented 1 year ago

This has been resolved in PHP 8.1+ but an issue before that, see the test at https://onlinephp.io/c/dbcd1 . The best solution for this is to extract the code within the Queue::push() to a separate job class and pass an instance of that to Queue::push() .

jonasraoni commented 1 year ago

Yep, the Laravel folks said the same and asked for a fix: https://github.com/laravel/framework/issues/46355 Perhaps I'll create a PR later (e.g. $job instanceof Closure).

Vitaliy-1 commented 1 year ago

Might be useful for this issue:

touhidurabir commented 1 year ago

@NateWr can you please review the PRs . The bulk emailing feature now handled by batch queue process with few enhancements .

NateWr commented 1 year ago

@Vitaliy-1 can you code review this? I worked on this back before all of @touhidurabir's work on jobs and your work on emails. I think you're more familiar with best practices here.

jonasraoni commented 1 year ago

A fix has already been added upstream, so the next release of Laravel 9 should include it :)

Vitaliy-1 commented 1 year ago

@touhidurabir, looks good! I also think that the separate API endpoint to process the queue isn't needed anymore as the concept has changed. I left a comment, can you check whether we duplicate registering of the database connector with this PR?

touhidurabir commented 1 year ago

@Vitaliy-1 I have replied to your reviews, please check those .

Vitaliy-1 commented 1 year ago

Thanks, @touhidurabir. Think that it's ready to be merged after the tests.