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

Upgrade extension Thanks to MW 1.43 | Pull all Fandom patches into Thanks 1.43 #11

Open
wants to merge 43 commits into
base: REL1_43
Choose a base branch
from

Conversation

maaartyyynaa
Copy link
Member

@maaartyyynaa maaartyyynaa commented Jan 24, 2025

Task: https://fandom.atlassian.net/browse/PLATFORM-10109

Please review if I didn't break anything while resolving conflicts for each cherry-pick. List of pulled changes:

Some thoughts: I have to say that upgrading this and fetching our patches into REL1_43 was a very painful process, as a result I've created this task - UGC-6220. I think we can either drop this fork and port all stuff into a custom extension or clean up this fork. I think it would be way easier to port changes if we introduce some minor changes (e.g., a dedicated file for our hooks - this way we won't need to resolve conflicts on the Hooks file in each commit but rather only conflicts in the extension.json; skipping changes related to the code sniffer, etc.). Additionally, I'd port all changes in a single commit.

Michał Chatłas and others added 23 commits January 24, 2025 10:21
In 8e3838e, we augmented Thanks to also
display "thank" link on Special:RecentChanges, and added logic to fetch
the revision corresponding to each RecentChanges entry because it is
necessary to render the link. However, this causes a DB lookup for each
RecentChanges entry and so quickly becomes expensive.

Instead, construct the revision object entirely using data already
stored in the RecentChanges entry itself, akin to what ChangesList does
internally. This avoids the problematic DB query altogether.
* @param array $revisions Array with two elements, either nulls or RevisionRecord objects for
* the two revisions that are being compared in the diff
*/
public static function onBeforeSpecialMobileDiffDisplay( &$output, $ctx, $revisions ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

check that because I don't know what that is, it's not in original commit dcbbee3

Choose a reason for hiding this comment

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

@maaartyyynaa maaartyyynaa force-pushed the REL1_43-fandom-changes branch 4 times, most recently from af835e9 to 29728d4 Compare January 24, 2025 12:34
@maaartyyynaa
Copy link
Member Author

@emkarcinos @kasperekt @dariuszpaluch @tmusial99
I've requested a review from all of you in this extension since we had a bunch of patches applied mostly by you here and I would really appreciate a code review (I got a lot of conflicts on each cherry-pick and I hope all is ported in proper way)

@maaartyyynaa
Copy link
Member Author

maaartyyynaa commented Jan 24, 2025

modules/ext.thanks.thank.js Outdated Show resolved Hide resolved
modules/ext.thanks.corethank.js Outdated Show resolved Hide resolved
includes/Hooks.php Outdated Show resolved Hide resolved
includes/Hooks.php Outdated Show resolved Hide resolved
extension.json Outdated Show resolved Hide resolved
modules/ext.thanks.mobilediff.js Outdated Show resolved Hide resolved
includes/Hooks.php Outdated Show resolved Hide resolved
Comment on lines +107 to +110
if ( !ThanksPermissions::checkUserPermissionsForThanks( RequestContext::getMain()->getOutput() ) ) {
return;
}

Choose a reason for hiding this comment

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

(non-blocking) This can be replaced with a hook invocation, that way Fandom code wouldn't be coupled with this class anymore. It's likely that the hook can be upstreamed, which will reduce the fork size in future versions.

Comment on lines +129 to +134
$out = RequestContext::getMain()->getOutput();

// [UGC-4257] Don't show thank links if user doesn't have specific permission
if ( !ThanksPermissions::checkUserPermissionsForThanks( $out ) ) {
return;
}

Choose a reason for hiding this comment

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

(non-blocking), same as above, invoke a hook

includes/Hooks.php Outdated Show resolved Hide resolved
modules/ext.thanks.corethank.js Outdated Show resolved Hide resolved
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.

8 participants