The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Opening up this followup issue from #538660: Move update manager upgrade process into new authorize.php file (and make it actually work) so that non-critical fixes to code style and API docs can be sorted after the main thing is committed, given in my timezone there's only three hours to code freeze, even if there's more like 20 in reality.
+++ includes/authorize.inc 14 Oct 2009 20:06:48 -0000
@@ -0,0 +1,232 @@
+ elseif ($authorize_filetransfer_default = variable_get('authorize_filetransfer_default', NULL));
No curly braces.
+++ includes/authorize.inc 14 Oct 2009 20:06:48 -0000
@@ -0,0 +1,232 @@
+ '#markup' => t('To continue please provide your server connection details'),
Missing a comma.
+++ includes/authorize.inc 14 Oct 2009 20:06:48 -0000
@@ -0,0 +1,232 @@
+ /*
+ * Here we create two submit buttons. For a JS enabled client, they will
+ * only ever see submit_process. However, if a client doesn't have JS
+ * enabled, they will see submit_connection on the first form (whden picking
+ * what filetranfer type to use, and submit_process on the second one (which
+ * leads to the actual operation)
+ */
While this isn't a function, one line summary would be nice, as would starting the comment with /**
+++ includes/authorize.inc 14 Oct 2009 20:06:48 -0000
@@ -0,0 +1,232 @@
+ '#name' => 'enter_connection_settings', // This is later changed in JS.
+ '#type' => 'submit',
+ '#value' => t('Enter connetion settings'), // As is this. @see authorize.js.
inline comments go on separate lines. Also a typo on 'connetion'.
+++ misc/authorize.js 14 Oct 2009 17:26:38 -0000
@@ -0,0 +1,29 @@
+ // Removes the float on the select box (used for non-JS interface)
+ if($('.connection-settings-update-filetransfer-default-wrapper').length > 0) {
+ console.log($('.connection-settings-update-filetransfer-default-wrapper'));
+ $('.connection-settings-update-filetransfer-default-wrapper').css('float', 'none');
+ }
+ // Hides the submit button for non-js users
Missing periods on these comments.
+++ modules/update/update.authorize.inc 15 Oct 2009 11:22:35 -0000
@@ -0,0 +1,302 @@
+ * The cannonical short name of the project being installed.
canonical
+++ modules/update/update.authorize.inc 15 Oct 2009 11:22:35 -0000
@@ -0,0 +1,302 @@
+ catch (UpdaterError $e) {
Should be Exception?
+++ modules/update/update.manager.inc 15 Oct 2009 01:26:59 -0000
@@ -0,0 +1,733 @@
+ // Filter out projects which are up2date already.
;)
+++ modules/update/update.manager.inc 15 Oct 2009 01:26:59 -0000
@@ -0,0 +1,733 @@
+ if ($needs_manual) {
+ // Since it won't be tableselect, #weight will confuse the table if it's
+ // defined, so just unset it (since the order doesn't really matter that
+ // much in the manual updates table, anyway).
+ unset($entry['#weight']);
+ }
How does it confuse it?
+++ modules/update/update.manager.inc 15 Oct 2009 01:26:59 -0000
@@ -0,0 +1,733 @@
+
+function update_manager_install_form(&$form_state) {
Everything in this file is missing a one-line summary of the function.
Probably some more small things, but nothing which should hold the patch up.
This review is powered by Dreditor.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 605344-2.update_manager_code_style_fixes.patch | 11.08 KB | dww |
Comments
Comment #1
dwwThanks, catch! Tagging for later.
Comment #2
dwwThis fixes everything catch pointed out, and everything of value that alexanderpas wrote at #538660-150: Move update manager upgrade process into new authorize.php file (and make it actually work), with the following exceptions:
A)
"No curly braces."
There are no statements to perform inside those curly braces. I don't see how
{}is more clear than;in this case. alexanderpas thought the whole code block was confusing:"I needed to look 3 times before understanding this construction, can't we just give the default value to variable_get() or something."
No, we can't (at least, shouldn't). That would imply that the setting we're testing has a default value. We want to be clear that the setting defaults to NULL, but it might be defined. If so, we have our value. Otherwise, we just grab the first FileTransfer option off the top of the list. I'm willing to consider rewriting this for better clarity, but not via the hack of putting the first option from the list as the default for the setting. I'm at a bit of a loss as to how to write it any more clear than it already is. Do we just need another code comment here or something?
B)
"While this isn't a function, one line summary would be nice, as would starting the comment with /**"
This just happens to be a multi-line code comment block inside a function. /** is specifically for doxygen comments at the tops of functions to document the function itself. I don't see how a separate 1-line summary is necessary in this case. It's just a 5-line code comment about how the form degrades without JS...
Otherwise, this fixes everything already pointed out, plus a few other bugs I found myself:
C) Using double quotes inside t() (e.g.
t("Something")) when all we need are single quotes (e.g.t('Something')).D) Same sorts of " vs. ' problems but for variable_get()
E)
if($('inside authorize.js instead ofif ($('.Comment #3
dwwComment #4
cweagansComment #5
dwwA few notes:
F) authorize.js is still pretty wonky, but that should be cleaned up when we fix all these forms and JS to work both with and without JS enabled: #609772: Impossible to extend the FileTransfer class system in contrib
G) There's one hunk of code-style fixes to includes/authorize.inc that's now included in the patch at #605920-10: Fix all buttons in update manager workflow to use "Sentence case" labels to avoid patches that conflict.
Comment #6
dries commentedCommitted to CVS HEAD. Thanks.