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

Random improvements #1

Merged
merged 1 commit into from Jun 17, 2014

Conversation

Projects
None yet
2 participants
@legoktm
Contributor

legoktm commented Jun 17, 2014

Mainly fixing documentation, adhering to MediaWiki code conventions, etc.

Left a few @fixme's where I saw things that didn't look right but wasn't sure on how to fix them.

Random improvements to the code
Mainly fixing documentation, adhering to MediaWiki code conventions, etc.

Left a few @fixme's where I saw things that didn't look right but
wasn't sure on how to fix them.
@@ -141,7 +132,7 @@ public static function onImageBeforeProduceHTML(
if ( self::$oldIDSet == true ) {
$history = $file->getHistory(
$limit = 1, $start = $articleDatetime);
/* $limit = */ 1, /* $start = */ $articleDatetime); // @fixme undefined variable

This comment has been minimized.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

I'm glad you found this. It should be self::$articleDatetime as set earlier by the onArticleViewHeader hook. I'm embarrassed this made it into the released version. Good thing I marked the TimeNegotiationForThumbnails option as EXPERIMENTAL.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

I'm glad you found this. It should be self::$articleDatetime as set earlier by the onArticleViewHeader hook. I'm embarrassed this made it into the released version. Good thing I marked the TimeNegotiationForThumbnails option as EXPERIMENTAL.

@@ -207,6 +198,7 @@ public static function onArticleViewHeader(
// let MediaWiki handle that case instead
if ( is_object( $revision ) ) {
// @fixme duplicate variable definition?
$articleDatetime = $revision->getTimestamp();
$articleDatetime = '00000000000000';

This comment has been minimized.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

Again, thanks. I'm going to remove line 211 in a follow-on commit after I accept your request. $articleDatetime should also be self::$articleDatetime.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

Again, thanks. I'm going to remove line 211 in a follow-on commit after I accept your request. $articleDatetime should also be self::$articleDatetime.

@@ -55,7 +55,6 @@ class MementoConfig {
*/
function __construct() {
global $wgMementoExcludeNamespaces;

This comment has been minimized.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

This global is used on line 87.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

This global is used on line 87.

This comment has been minimized.

@legoktm

legoktm Jun 17, 2014

Contributor

The global declaration is present twice, so I removed one of them. See line 62.

@legoktm

legoktm Jun 17, 2014

Contributor

The global declaration is present twice, so I removed one of them. See line 62.

This comment has been minimized.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

Now I see. Thank you.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

Now I see. Thank you.

* @param $title - Title object of the page
* @param $parser - Parsger object of the page
* @param $id - the revision id of the page
* @fixme make this compatible with parser cache

This comment has been minimized.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

We're hoping you can give some input and guidance on what issues we may have with the parser cache. We tested this manually and it worked, but it was a single server installation with multiple MediaWikis.

@shawnmjones

shawnmjones Jun 17, 2014

Contributor

We're hoping you can give some input and guidance on what issues we may have with the parser cache. We tested this manually and it worked, but it was a single server installation with multiple MediaWikis.

This comment has been minimized.

@legoktm

legoktm Jun 18, 2014

Contributor

The problem is $parser->getUser()->getRequest(). You are making the result of the parser dependent on the values provided in the WebRequest, but don't split the parsercache based on those.

@legoktm

legoktm Jun 18, 2014

Contributor

The problem is $parser->getUser()->getRequest(). You are making the result of the parser dependent on the values provided in the WebRequest, but don't split the parsercache based on those.

This comment has been minimized.

@shawnmjones

shawnmjones Jun 18, 2014

Contributor

So should I disable the cache afterward with $parser->disableCache()?

@shawnmjones

shawnmjones Jun 18, 2014

Contributor

So should I disable the cache afterward with $parser->disableCache()?

This comment has been minimized.

@legoktm

legoktm Jun 18, 2014

Contributor

Yeah, that would fix the problem right now with users getting possibly corrupted data, but the real solution would be to have this work with parser cache. I'm not really sure how to do that.

@legoktm

legoktm Jun 18, 2014

Contributor

Yeah, that would fix the problem right now with users getting possibly corrupted data, but the real solution would be to have this work with parser cache. I'm not really sure how to do that.

shawnmjones added a commit that referenced this pull request Jun 17, 2014

Merge pull request #1 from legoktm/fix
Random improvements, follow on commit coming afterwards to make some additional improvements based on this input

@shawnmjones shawnmjones merged commit 124c153 into mementoweb:master Jun 17, 2014

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