mfelczak/bepress#28 Add OJS 3.3 support #29
Conversation
|
Hi @mfelczak, this is ready for review. Could you have a look. I've tested it with both PHP 7.4 and PHP 8.0. Thanks! |
|
Hey @jonasraoni, would you be able to do a quick review of this? Thanks! |
| // FIXME: This attaches the associated user to the request and is a workaround for no users being present | ||
| // when running CLI tools. This assumes that given the username supplied should be used as the | ||
| // authenticated user. To revisit later. | ||
| Registry::set('user', $editor); |
There was a problem hiding this comment.
I had to add this workaround and a setContext() to the NativeImportExportPlugin, perhaps we should move this into the ImportExportPlugin later 🤔
|
|
||
| foreach ($importArticles as $entry) { | ||
| $articlePath = $issuePath . '/' . $entry; | ||
| if (!is_dir($articlePath) || preg_match('/^\./', $entry)) continue; | ||
| if (!is_dir($articlePath) || preg_match('/^\./', $entry) || !$entry) continue; |
There was a problem hiding this comment.
I think it would be better to fix the loop with an extra variable (e.g. for ($importArticles = []; $path = readdir($articleHandle); $importArticles[] = $path);), but that's just my preference 😁
There was a problem hiding this comment.
The preg_match('/^\./', $entry) could ignore a good folder (e.g. .xxx), but if I remember well this plugin expects folders to be numbered, right? Then perhaps it's better to check if the entry is a valid number, otherwise emit a warning.
There was a problem hiding this comment.
I think it would be better to fix the loop with an extra variable (e.g.
for ($importArticles = []; $path = readdir($articleHandle); $importArticles[] = $path);), but that's just my preference 😁
@jonasraoni, just curious: what would that look like in this case? That's not a pattern I've typically used.
There was a problem hiding this comment.
I thought you've added the !$entry to cover the case of readdir() returning false in the last iteration, if that's not the purpose, then ignore my comment =]
what would that look like in this case?
I've left a piece of untested code in the comment to fix the loop, that should probably be applied to the other places.
In fact, here I would replace the readdir() by the FilesystemIterator, which will skip the dot folders automatically \o/
|
|
||
| /** @var GenreDAO $genreDao */ | ||
| $genreDao = DAORegistry::getDAO('GenreDAO'); | ||
| $genre = $genreDao->getByKey('SUBMISSION', $this->_journal->getId()); |
There was a problem hiding this comment.
I think this can be cached out of the loop.
There was a problem hiding this comment.
@ewhanson, with the exception of this comment: https://github.com/mfelczak/bepress/pull/29/files#r939505772, the others are just near useless notes 😁
No description provided.