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

Conversation

freephile
Copy link

Jeroen, this is my first attempt.

I know I can't call updateCacheExpiry() on a null parser object (see error) but I'm not sure what to do... I was going to check hooks and see if it needs to be implemented in a different hook function besides onParserFirstCallInit()

Error from line 23 of /var/www/html/extensions/ExternalContent/src/EntryPoints/MediaWikiHooks.php: Call to a member function updateCacheExpiry() on null

Can you look at my efforts and give me any feedback?

Aside: I would normally do this in a feature branch, but had some trouble getting VSCode in a container to work with GitHub, and the terminal was constrained (no prompts) so I had to do some docker file copies back to my host machine and then still ended up pushing to 'master' in my forked repo :-(

@@ -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

} 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?

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