🧹 [code health] Refactor procedural bookmarklet logic in tools.php#222
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| function yourls_get_bookmarklet_js( $type, $base_bookmarklet ) { | ||
| switch ( $type ) { | ||
| case 'STANDARD_SIMPLE': | ||
| return <<<STANDARD_SIMPLE | ||
| // Simple Standard Bookmarklet (new page, no keyword asked) | ||
| var d = document, | ||
| w = window, | ||
| enc = encodeURIComponent, | ||
| e = w.getSelection, | ||
| k = d.getSelection, | ||
| x = d.selection, | ||
| s = (e ? e() : (k) ? k() : (x ? x.createRange().text : 0)), | ||
| s2 = ((s.toString() == '') ? s : enc(s)), | ||
| f = '$base_bookmarklet', | ||
| l = d.location.href, | ||
| ups = l.match( /^[a-zA-Z0-9\+\.-]+:(\/\/)?/ )[0], | ||
| ur = l.split(new RegExp(ups))[1], | ||
| ups = ups.split(/\:/), | ||
| p = '?up='+enc(ups[0]+':')+'&us='+enc(ups[1])+'&ur='+enc(ur)+'&t='+enc(d.title)+'&s='+s2, | ||
| u = f + p; | ||
| try { | ||
| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u)) l.href = u; | ||
| }; | ||
| if (/Firefox/.test(navigator.userAgent)) setTimeout(a, 0); | ||
| else a(); | ||
| } | ||
| void(0); | ||
| STANDARD_SIMPLE; | ||
|
|
||
| case 'POPUP_SIMPLE': | ||
| return <<<POPUP_SIMPLE | ||
| // Simple Popup (in-page popup dialog, no keyword asked) | ||
| var d = document, | ||
| sc = d.createElement('script'), | ||
| l = d.location.href, | ||
| enc = encodeURIComponent, | ||
| ups = l.match( /^[a-zA-Z0-9\+\.-]+:(\/\/)?/ )[0], | ||
| ur = l.split(new RegExp(ups))[1], | ||
| ups = ups.split(/\:/), | ||
| p = '?up='+enc(ups[0]+':')+'&us='+enc(ups[1])+'&ur='+enc(ur)+'&t='+enc(d.title); | ||
| window.yourls_callback = function (r) { | ||
| if (r.short_url) { | ||
| prompt(r.message, r.short_url); | ||
| } else { | ||
| alert('An error occurred: ' + r.message); | ||
| } | ||
| }; | ||
| sc.src = '$base_bookmarklet' + p + '&jsonp=yourls'; | ||
| void(d.body.appendChild(sc)); | ||
| POPUP_SIMPLE; | ||
|
|
||
| case 'CUSTOM_STANDARD': | ||
| return <<<CUSTOM_STANDARD | ||
| // Custom Standard (new page, prompt for a keyword) | ||
| var d = document, | ||
| enc = encodeURIComponent, | ||
| w = window, | ||
| e = w.getSelection, | ||
| k = d.getSelection, | ||
| x = d.selection, | ||
| s = (e ? e() : (k) ? k() : (x ? x.createRange().text : 0)), | ||
| s2 = ((s.toString() == '') ? s : enc(s)), | ||
| f = '$base_bookmarklet', | ||
| l = d.location.href, | ||
| ups = l.match( /^[a-zA-Z0-9\+\.-]+:(\/\/)?/ )[0], | ||
| ur = l.split(new RegExp(ups))[1], | ||
| ups = ups.split(/\:/), | ||
| k = prompt("Custom URL"), | ||
| k2 = (k ? '&k=' + k : ""), | ||
| p = '?up='+enc(ups[0]+':')+'&us='+enc(ups[1])+'&ur='+enc(ur)+'&t='+enc(d.title)+'&s='+s2 + k2, | ||
| u = f + p; | ||
| if (k != null) { | ||
| try { | ||
| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u)) l = u; | ||
| }; | ||
| if (/Firefox/.test(navigator.userAgent)) setTimeout(a, 0); | ||
| else a(); | ||
| } | ||
| void(0) | ||
| } | ||
| CUSTOM_STANDARD; | ||
|
|
||
| case 'CUSTOM_POPUP': | ||
| return <<<CUSTOM_POPUP | ||
| // Custom Popup (prompt for a keyword + on-page popup) | ||
| var d = document, | ||
| l = d.location.href, | ||
| k = prompt('Custom URL'), | ||
| enc = encodeURIComponent, | ||
| ups = l.match( /^[a-zA-Z0-9\+\.-]+:(\/\/)?/ )[0], | ||
| ur = l.split(new RegExp(ups))[1], | ||
| ups = ups.split(/\:/), | ||
| p = '?up='+enc(ups[0]+':')+'&us='+enc(ups[1])+'&ur='+enc(ur)+'&t='+enc(d.title); | ||
| sc = d.createElement('script'); | ||
| if (k != null) { | ||
| window.yourls_callback = function (r) { | ||
| if (r.short_url) { | ||
| prompt(r.message, r.short_url); | ||
| } else { | ||
| alert('An error occurred: ' + r.message); | ||
| } | ||
| }; | ||
| sc.src = '$base_bookmarklet' + p + '&k=' + k + '&jsonp=yourls'; | ||
| void(d.body.appendChild(sc)); | ||
| } | ||
| CUSTOM_POPUP; | ||
|
|
||
| case 'FACEBOOK': | ||
| return <<<FACEBOOK | ||
| // Share on Facebook | ||
| var d = document, | ||
| enc = encodeURIComponent, | ||
| f = '$base_bookmarklet', | ||
| l = d.location.href, | ||
| ups = l.match( /^[a-zA-Z0-9\+\.-]+:(\/\/)?/ )[0], | ||
| ur = l.split(new RegExp(ups))[1], | ||
| ups = ups.split(/\:/), | ||
| p = '?up=' + enc(ups[0]+':') + '&us=' + enc(ups[1]) + '&ur=' + enc(ur) + '&t=' + enc(d.title) + '&share=facebook', | ||
| u = f + p; | ||
| try { | ||
| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!window.open(u,'Share','width=500,height=340,left=100','_blank')) l.href = u; | ||
| }; | ||
| if (/Firefox/.test(navigator.userAgent)) setTimeout(a, 0); | ||
| else a(); | ||
| } | ||
| void(0); | ||
| FACEBOOK; | ||
|
|
||
| case 'TWITTER': | ||
| return <<<TWITTER | ||
| // Share on Twitter | ||
| var d = document, | ||
| w = window, | ||
| enc = encodeURIComponent, | ||
| e = w.getSelection, | ||
| k = d.getSelection, | ||
| x = d.selection, | ||
| s = (e ? e() : (k) ? k() : (x ? x.createRange().text : 0)), | ||
| s2 = ((s.toString() == '') ? s : '%20%22' + enc(s) + '%22'), | ||
| f = '$base_bookmarklet', | ||
| l = d.location.href, | ||
| ups = l.match( /^[a-zA-Z0-9\+\.-]+:(\/\/)?/ )[0], | ||
| ur = l.split(new RegExp(ups))[1], | ||
| ups = ups.split(/\:/), | ||
| p = '?up=' + enc(ups[0]+':') + '&us=' + enc(ups[1]) + '&ur='+enc(ur) + '&t=' + enc(d.title) + s2 + '&share=twitter', | ||
| u = f + p; | ||
| try { | ||
| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u,'Share','width=780,height=265,left=100','_blank')) l = u; | ||
| }; | ||
| if (/Firefox/.test(navigator.userAgent)) setTimeout(a, 0); | ||
| else a(); | ||
| } | ||
| void(0); | ||
| TWITTER; | ||
|
|
||
| case 'TUMBLR': | ||
| return <<<TUMBLR | ||
| // Share on Tumblr | ||
| var d = document, | ||
| w = window, | ||
| enc = encodeURIComponent, | ||
| share = 'tumblr', | ||
| e = w.getSelection, | ||
| k = d.getSelection, | ||
| x = d.selection, | ||
| s = (e ? e() : (k) ? k() : (x ? x.createRange().text : 0)), | ||
| s2 = ((s.toString() == '') ? s : '%20%22' + enc(s) + '%22'), | ||
| f = '$base_bookmarklet', | ||
| l = d.location.href, | ||
| ups = l.match( /^[a-zA-Z0-9\+\.-]+:(\/\/)?/ )[0], | ||
| ur = l.split(new RegExp(ups))[1], | ||
| ups = ups.split(/\:/), | ||
| p = '?up=' + enc(ups[0]+':') + '&us=' + enc(ups[1]) + '&ur='+enc(ur) + '&t=' + enc(d.title) + '&s=' + s2 + '&share=tumblr', | ||
| u = f + p; | ||
| try { | ||
| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u,'Share','width=450,height=450,left=430','_blank')) l = u; | ||
| }; | ||
| if (/Firefox/.test(navigator.userAgent)) setTimeout(a, 0); | ||
| else a(); | ||
| } | ||
| void(0); | ||
| TUMBLR; | ||
|
|
||
| default: | ||
| return ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
Code Review
This pull request refactors procedural bookmarklet generation by extracting long JavaScript heredocs from admin/tools.php into a central helper function yourls_get_bookmarklet_js in includes/functions-html.php. However, several issues were identified in the extracted JavaScript code: in multiple bookmarklets, attempting to redirect the browser by assigning to or modifying properties of l (which is a string primitive of d.location.href) will fail, and should be replaced with w.location.href. Additionally, a premature semicolon in the CUSTOM_POPUP bookmarklet terminates a var declaration, causing sc to leak as an implicit global variable.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u)) l.href = u; |
There was a problem hiding this comment.
In the STANDARD_SIMPLE bookmarklet, l is defined as d.location.href (a string primitive). Attempting to set l.href = u will fail to redirect the browser because you cannot set properties on a string primitive. To ensure the fallback redirection works correctly when window.open fails (e.g., due to popup blockers), use w.location.href instead.
if (!w.open(u)) w.location.href = u;| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u)) l = u; |
There was a problem hiding this comment.
In the CUSTOM_STANDARD bookmarklet, l is defined as d.location.href (a string primitive). Reassigning l = u only updates the local variable and does not redirect the browser. To ensure the fallback redirection works correctly when window.open fails, use w.location.href instead.
if (!w.open(u)) w.location.href = u;| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!window.open(u,'Share','width=500,height=340,left=100','_blank')) l.href = u; |
There was a problem hiding this comment.
In the FACEBOOK bookmarklet, l is defined as d.location.href (a string primitive). Attempting to set l.href = u will fail to redirect the browser. Use window.location.href instead to ensure the fallback redirection works correctly.
if (!window.open(u,'Share','width=500,height=340,left=100','_blank')) window.location.href = u;| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u,'Share','width=780,height=265,left=100','_blank')) l = u; |
There was a problem hiding this comment.
In the TWITTER bookmarklet, l is defined as d.location.href (a string primitive). Reassigning l = u only updates the local variable and does not redirect the browser. Use w.location.href instead to ensure the fallback redirection works correctly.
if (!w.open(u,'Share','width=780,height=265,left=100','_blank')) w.location.href = u;| throw ('ozhismygod'); | ||
| } catch (z) { | ||
| a = function () { | ||
| if (!w.open(u,'Share','width=450,height=450,left=430','_blank')) l = u; |
There was a problem hiding this comment.
In the TUMBLR bookmarklet, l is defined as d.location.href (a string primitive). Reassigning l = u only updates the local variable and does not redirect the browser. Use w.location.href instead to ensure the fallback redirection works correctly.
if (!w.open(u,'Share','width=450,height=450,left=430','_blank')) w.location.href = u;| p = '?up='+enc(ups[0]+':')+'&us='+enc(ups[1])+'&ur='+enc(ur)+'&t='+enc(d.title); | ||
| sc = d.createElement('script'); |
There was a problem hiding this comment.
The semicolon at the end of line 1348 terminates the var declaration statement. As a result, sc on line 1349 is declared without var, leaking it as an implicit global variable. Changing the semicolon to a comma will keep it within the var statement.
p = '?up='+enc(ups[0]+':')+'&us='+enc(ups[1])+'&ur='+enc(ur)+'&t='+enc(d.title),
sc = d.createElement('script');
🎯 What: Extracted large JavaScript heredoc blocks from
admin/tools.phpinto a new helper functionyourls_get_bookmarklet_jsinincludes/functions-html.php.💡 Why: Improves the readability and maintainability of the
admin/tools.phpscript by moving complex and repetitive procedural data definitions into an encapsulated helper function.✅ Verification: Ran a targeted standalone script to ensure syntax correctness, as the local db tests lack configuration. Manual review of generated PHP files indicates no missing elements. Lint checks passed.
✨ Result:
admin/tools.phpis now significantly cleaner, more readable, and easier to modify in the future.PR created automatically by Jules for task 11943119923844353995 started by @projectedanx