Repository navigation
Conversation
Splinter is unmaintained and should not ship with Bugzilla 6. Remove extensions/Splinter and every reader of its splinter_base param: the /review route, the Splinter Review link on patch attachments in BugModal, the MyDashboard flag link, the Review extension's page hook and request badge link, and the param export in the global header. Patch links in the review badge now fall back to the attachment details page, as other attachments already do. update_params does not drop params that are no longer defined, so upgraded installs keep a stale splinter_base value in data/params. Nothing reads it any more, so it is harmless and is left in place.
mrenvoize
left a comment
There was a problem hiding this comment.
Approving. git grep finds no remaining splinter references outside extensions/Splinter (apart from a binary symbol in SecureMail/bin/tct), there is no DB update to clean up, and removing the /review route (App.pm:205) is consistent. Notes for the release notes: old /review links will 404, and a stale splinter_base stays in existing params files (harmless).
mrenvoize
left a comment
There was a problem hiding this comment.
Changing my earlier approval to changes requested. My first review was a static read, and I said no references to Splinter remained outside the extension. That was true of the word, but not of the links. I have now run this in Docker against the PR head (047303b) and found one regression.
Dead "Review" link in the RequestNagger reminder emails. Both templates add a link for every patch request that points at the /review route this PR removes:
extensions/RequestNagger/template/en/default/email/request_nagging-user.html.tmpl:64:<a href=".../review?bug=...&attachment=...">Review</a>extensions/RequestNagger/template/en/default/email/request_nagging-user.txt.tmpl:34:Review: .../review?bug=...&attachment=...
I rendered both real templates with this PR applied, for a patch request. Both still output http://.../review?bug=1&attachment=1, and that URL now returns 404, so reminder recipients would get a dead link on every overdue patch request. The earlier "no splinter references" check missed it because these links never mention the word.
Suggested fix, matching what the PR already does for the Review badge:
- HTML: remove the
| <a href="...review?bug=...">Review</a>line. "Details" and "Diff" are already beside it. - Plain text: replace the
Review:line withDetails: [% urlbase %]attachment.cgi?id=[% request.attachment.id %]&action=edit, still inside the existingIF request.attachment && request.attachment.ispatchblock.
Everything else passed, so with that fixed I'm happy to approve:
- The bug page, MyDashboard and the home page load with no console errors and no Splinter links, and
splinter_baseis gone from the page's JS params. - The MyDashboard flags table renders the review request as plain text.
- Called directly, the rewritten Review badge
get_linklinks a single patch to the attachment details page and a duplicate group to the bug, with no errors. /reviewreturns 404, as intended.- Upgrade case: with a stale
splinter_baseleft indata/params, the app starts and those pages all return 200. - All 32 test files pass (14,067 tests).
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1545443
Splinter is unmaintained and should not ship with Bugzilla 6. This removes extensions/Splinter and every reader of its splinter_base param:
Upgrade note: update_params does not drop params that are no longer defined, so upgraded installs keep a stale splinter_base value in data/params. Every reader is removed, so it is harmless; I left it rather than adding param cleanup code.
Tested locally in Docker, on a database that still has splinter_base set (the upgrade case):