Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: add Cache expiry settings #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,25 @@ $wgExternalContentBasicAuthCredentials = [
The above example shows how you can get credentials from ENV vars, which might be preferred over
storing them as plaintext in [LocalSettings.php].

### Disable Caching

Variable: `$wgExternalContentDisableCache`

Default: `false`

Note: changing this to `true` is NOT advised. It will have a negative effect on performance since every page view where External Content
is used will require an external file fetch.

### Default Content Cache Expiry

Variable: `$wgExternalContentDefaultExpiry`

Default: `86400` (24 hours)

Example: `604800` (1 week) `0` (no caching)

See also: run the [maintenance script](#refreshing-external-content) to expire the cache for all External Content

### Connection details

Content of files is fetched via MediaWiki's native HTTP client. This process is affected by
Expand Down
10 changes: 9 additions & 1 deletion extension.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "External Content",
"version": "1.3.0",
"version": "1.4.0",
"license-name": "GPL-2.0-or-later",

"author": [
Expand Down Expand Up @@ -39,6 +39,14 @@
"ExternalContentBasicAuthCredentials": {
"description": "Per-domain Basic Auth credentials.",
"value": []
},
"ExternalContentDisableCache": {
"description": "Disable caching of external content. '''Use with caution''' as this forces a remote file fetch on every page view where this extension is used!",
"value": false
},
"ExternalContentDefaultExpiry": {
"description": "Default content expiry in seconds (86400 = 1 day). This will only have an effect if lower than MediaWiki's default sitewide [https://www.mediawiki.org/wiki/Manual:$wgParserCacheExpireTime $wgParserCacheExpireTime]",
"value": 86400
}
},

Expand Down
2 changes: 2 additions & 0 deletions src/EmbedExtensionFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class EmbedExtensionFactory {
'ExternalContentFileExtensionWhitelist' => [ 'md' ],
'wgExternalContentEnableEmbedFunction' => true,
'wgExternalContentEnableBitbucketFunction' => true,
'ExternalContentDefaultExpiry' => 86400,
Copy link
Author

Choose a reason for hiding this comment

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

should these be prefixed with 'wg'?

Copy link
Member

Choose a reason for hiding this comment

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

With wg

I made it consistent in #18

'ExternalContentDisableCache' => false,
];

protected static ?self $instance;
Expand Down
24 changes: 24 additions & 0 deletions src/EntryPoints/MediaWikiHooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@
final class MediaWikiHooks {

public static function onParserFirstCallInit( Parser $parser ): void {
if ( $parser ) {
if ( self::cacheIsDisabled() ) {
$parser->getOutput()->updateCacheExpiry( 0 );
// We only need to set the External Content cache expiry if it is lower than the sitewide expiry
} elseif ( is_int( self::getCacheExpiry() ) && !self::hasReducedExpiry() ) {
$parser->getOutput()->updateCacheExpiry( self::getCacheExpiry() );
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The if $parser is not needed. You can tell by the function signature you always get one.

That said, this code should be elsewhere. onParserFirstCallInit gets called always, even if there is no embedded content on a page.

Copy link
Member

Choose a reason for hiding this comment

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

See the BitbucketFunction and EmbedFunction classes at https://github.com/ProfessionalWiki/ExternalContent/tree/master/src/EntryPoints for the code that handles calls to these parser functions.

Copy link
Author

@freephile freephile Feb 10, 2022

Choose a reason for hiding this comment

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

So I "don't repeat myself", should the code be added to the EmbedExtensionFactory, and then referenced from the entrypoints?

if ( self::embedFunctionIsEnabled() ) {
$parser->setFunctionHook(
'embed',
Expand All @@ -32,6 +40,22 @@ public static function onParserFirstCallInit( Parser $parser ): void {
}
}

/**
* Compatibility shim for hasReducedExpiry() coming in 1.37
*/
private static function hasReducedExpiry(): bool {
$parserCacheExpireTime = MediaWikiServices::getInstance()->getMainConfig()->get( 'ParserCacheExpireTime' );
return self::getCacheExpiry() < $parserCacheExpireTime;
}

private static function cacheIsDisabled(): bool {
return MediaWikiServices::getInstance()->getMainConfig()->get( 'ExternalContentDisableCache' );
}

private static function getCacheExpiry(): int {
return MediaWikiServices::getInstance()->getMainConfig()->get( 'ExternalContentDefaultExpiry' );
}

/**
* @psalm-suppress MixedInferredReturnType
* @psalm-suppress MixedReturnStatement
Expand Down