#50 - 3.4 compatibility #51
Conversation
|
Thanks, @mariuszsienkiewicz! @touhidurabir, could you review this one? |
| **/ | ||
| function getForcedCredentials() { | ||
| **/ | ||
| function getForcedCredentials() |
There was a problem hiding this comment.
How about adding proper return type hint as starting from 3.4 we start following more strict type implementation like adding proper param type and return type . It can be here and anywhere possible .
| function getActions($request, $verb) | ||
| { | ||
| $router = $request->getRouter(); | ||
| import('lib.pkp.classes.linkAction.request.AjaxModal'); |
There was a problem hiding this comment.
we can remove these import method calls to load class as we have proper namespace feature now
There was a problem hiding this comment.
Yes, that should be part of adapting any code forward from 3.3 (along with the other things in the 3.4 Release Notebook).
| } | ||
|
|
||
| /** | ||
| * Low-budget mock class for \bsobbe\ithenticate\Ithenticate -- Replace the | ||
| * constructor above with this class name to log API usage instead of | ||
| * interacting with the iThenticate service. | ||
| */ | ||
| class TestIthenticate { | ||
| public function __construct($username, $password) { | ||
| class TestIthenticate |
There was a problem hiding this comment.
Instead of having this TestIthenticate in the PlagiarismPlugin.php class file, it's better to have it in own file with proper namespace .
| error_log("Constructing iThenticate: $username $password"); | ||
| } | ||
|
|
||
| public function fetchGroupList() { | ||
| public function fetchGroupList() | ||
| { | ||
| error_log('Fetching iThenticate group list'); | ||
| return array(); |
There was a problem hiding this comment.
As we are using newer array syntax [], better to keep that same format everywhere like return [] ;
| $this->_plugin->updateSetting($this->_contextId, 'ithenticateUser', trim($this->getData('ithenticateUser'), "\"\';"), 'string'); | ||
| $this->_plugin->updateSetting($this->_contextId, 'ithenticatePass', trim($this->getData('ithenticatePass'), "\"\';"), 'string'); | ||
| $this->_plugin->updateSetting($this->_contextId, 'ithenticateUser', trim($this->getData('ithenticateUser'), "\"\';"), 'string'); | ||
| $this->_plugin->updateSetting($this->_contextId, 'ithenticatePass', trim($this->getData('ithenticatepass'), "\"\';"), 'string'); |
There was a problem hiding this comment.
It should be be ithenticatePass instead of ithenticatepass that is with capital P .
| { | ||
|
|
||
| /** @var PlagiarismPlugin $plugin */ | ||
| private $plugin; |
There was a problem hiding this comment.
perhaps better to use exact property type hint like private PlagiarismPlugin $plugin ?
| { | ||
| $events->listen( | ||
| SubmissionSubmitted::class, | ||
| PlagiarismSubmissionSubmitListener::class |
There was a problem hiding this comment.
instead of specifying the exact class name here, we can do it like
$events->listen(
SubmissionSubmitted::class,
[static::class, 'handle']
)
| public function sendErrorMessage($submissionid, $message) { | ||
| **/ | ||
| public function sendErrorMessage($submissionid, $message) | ||
| { | ||
| $request = Application::get()->getRequest(); | ||
| $context = $request->getContext(); | ||
| import('classes.notification.NotificationManager'); |
There was a problem hiding this comment.
|
@mariuszsienkiewicz sorry for the late, I have added few comments . can you check those ? |
Resolves: #50.
I tried to maintain the existing plugin structure. The main changes are that the callback function has been renamed to
sendSubmissionFilesand is called from thePlagiarismSubmissionSubmitListenerclass. Maybe it would be better to move the entire method to the listener, but as I mentioned, I didn't want to make too significant changes to the structure so I left it like that.