Skip to content

3.4 updates - #21

Merged
trp89 merged 9 commits into
masterfrom
3.4_Updates
Dec 18, 2024
Merged

3.4 updates#21
trp89 merged 9 commits into
masterfrom
3.4_Updates

Conversation

@trp89

@trp89 trp89 commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@ctgraham ctgraham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One potentially unintentional change, low priority.

Comment thread css/clamav.css Outdated
@trp89
trp89 requested review from ctgraham and wopsononock October 28, 2024 19:33
@trp89

trp89 commented Oct 28, 2024

Copy link
Copy Markdown
Contributor Author

@ctgraham and/or @wopsononock I just wanted another quick onceover to make sure these are good to merge the pull request

@ctgraham ctgraham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like _getBackwardsCompatibleContext() is now just cruft and can be removed.

Comment thread ClamavPlugin.php Outdated
@trp89
trp89 merged commit 64ec143 into master Dec 18, 2024

@ctgraham ctgraham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires confirmation.

Comment thread ClamavPlugin.php
case 'settings':
$contextID = (!is_null($this->_getBackwardsCompatibleContext()) ? $this->_getBackwardsCompatibleContext()->getId() : PKPApplication::CONTEXT_SITE);
$context = Application::get()->getRequest()->getContext();
$contextID = $context->getId();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some risk to this change.

Previously, we checked to see if getContext() returned null, and if so, substituted PKPApplication::CONTEXT_SITE for the context id. The current code presumes that $context must not be null.

This can be checked by trying to manage the plugin from the Site level, rather than from the journal level. Or, we could re-introduce the if statement presumptively.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants