Merge lp:~edb/quam-plures/plugins_list_available into lp:quam-plures
- plugins_list_available
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 7638 |
Proposed branch: | lp:~edb/quam-plures/plugins_list_available |
Merge into: | lp:quam-plures |
Diff against target: |
222 lines (+76/-105) 1 file modified
qp_inc/plugins/views/_plugin_list_available.view.php (+76/-105) |
To merge this branch: | bzr merge lp:~edb/quam-plures/plugins_list_available |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tilman Blumenbach (community) | Approve | ||
EdB | Needs Resubmitting | ||
Review via email: mp+73260@code.launchpad.net |
Commit message
Description of the change
http://
4) On the "add a plugin" page it says "Plugins available for installation" then lists ALL plugins even if they can not have another installation. I think we should make that page do what it says, and list only the plugins that can be installed.
There is a cosmetic issue due to I did not remove creating table sections for sub-groups, which I did not do because sub-groups are going away in another branch.
going through emails, thought I'd pause at this one for a quick answer. The last bit - the "where to get new plugins" is completely inaccurate. It might be a nice idea, but until there is an actual place with actual plugins it makes no sense to create a link there.
I'll go back to reading stuff though, and trying to get the truck to run, and then get into all the nice feedback you're providing!
This one is next but I'm very tired. Anyway lemme see... The "no subgroup" branch will cure the issue of phantom subgroups. I commented on that as a cosmetic issue and don't see any reason to fix something that will cure itself.
Not closing a table is bad so needs fixing for sure.
I thought about removing the bit that doesn't display the available events but figured it didn't hurt to have it in there, so I'll make that be right even though it is commented out.
I'll have to look at the count_regs() thing. I know it is duplicated but I'm pretty sure I had a fairly good reason. Like an issue during development that made me use the same code to reach the same conclusion, but since it is needs fixing it doesn't matter that much.
No interest in the dead end link. I guess I could restore it and comment it out so one day it is easily restored, but it goes to nothing so why bother?
Tilman Blumenbach (tblue) wrote : | # |
> I commented on that as a cosmetic issue and don't see any reason to fix something that will cure itself.
Point taken! :)
> I'll have to look at the count_regs() thing. I know it is duplicated but I'm pretty sure I had a fairly good reason.
Okay, I didn't actually try to remove it, I just saw that it was duplicated.
> No interest in the dead end link.
Yes, the link may be wrong and should be removed, but isn't the "drop plugins into that directory" bit still helpful? I guess we could remove it and explain plugin installation in the to-be-written documentation, though.
I am adding core updates AND retesting everything I have in merge just to be sure, but it is taking a long time due to many conflicts this time around. Will send another comment when I got them back up to speed.
Okay cool. I can copy/paste many times, and I got rid of the second whatever it was line that didn't need to be there. Also made sure it didn't break the new "multiple instances" feature we got in there.
good to go :)
OT: I think it is Lee's turn to do a bunch of core merging.
Tilman Blumenbach (tblue) wrote : | # |
Looks better now, BUT:
- The "[Install]" link now looks like this: "[ Install]" (note the added space at the beginning). This is because in diff line 146, there now is a newline after the opening <a> tag and before the <?php bit. If you make it look like this:
[<a href="<?php echo $dispatcher ?>?ctrl=
then the Install link should look good again. :-)
- I just noticed that because we now check the user's perms and whether a plugin can be installed at the beginning of the big while loop, the old check on diff ll. 141 is redundant and could be removed.
Fixed, but not the way you think. It seems silly to exit PHP just to go back into it 3 times in the next line. So now we don't drop out of PHP just to do a few characters :) Unfortunately there are many cases where we do that. One less now, but only one less.
Tilman Blumenbach (tblue) wrote : | # |
Okay, looks fine. :)
Could you look into removing the second check I mentioned above? It doesn't hurt, but it's just not necessary. Removing it is easy as well.
Tilman Blumenbach (tblue) wrote : | # |
Well, all good then!
Tilman Blumenbach (tblue) wrote : | # |
...nope.
Parse error: syntax error, unexpected T_BOOLEAN_OR, expecting ',' or ')' in /home/tilman/
It's a incorrectly placed brace.
Tilman Blumenbach (tblue) wrote : | # |
By the way, you can just remove the whole if statement. Everything that it checks is already being checked above, just after the while loop begins.
HOPEFULLY this is the last time it gets fixed
Tilman Blumenbach (tblue) wrote : | # |
Okay, I'm SO sorry it's still broken. And it's not just me saying that because there's some code I don't like, it's because I'm getting a Red Page of Death when trying to install a plugin (with a SQL error).
Diff line 195:
echo '[<a href='.
There's a missing double quote right after href= (which should be href="), causing the browser to think I want to install a plugin with the classname <whatever>" (with a trailing double quote).
Actually, this shows that we have a big fat SQL injection vulnerability in Plugins_
Once you have fixed the missing quote, everything should FINALLY be okay. Sigh.
I'm tired of this. Either you're happy or I'm gonna undo all the bullshit until it is where it was when I knew it was 100% fully operational and did not cause any errors.
Removing "extra" checks is fucking bullshit coming from someone willing to bloat a style sheet with useless comments.
Fuckit.
Tilman Blumenbach (tblue) wrote : | # |
Does somebody else want to review this one? I usually merge branches after they have been approved by at least two reviewers.
Lee Turner (leeturner) wrote : | # |
This one is next on my list
Preview Diff
1 | === modified file 'qp_inc/plugins/views/_plugin_list_available.view.php' | |||
2 | --- qp_inc/plugins/views/_plugin_list_available.view.php 2011-09-04 22:27:27 +0000 | |||
3 | +++ qp_inc/plugins/views/_plugin_list_available.view.php 2011-09-13 21:46:49 +0000 | |||
4 | @@ -40,27 +40,19 @@ | |||
5 | 40 | * @var Plugins | 40 | * @var Plugins |
6 | 41 | */ | 41 | */ |
7 | 42 | global $admin_Plugins; | 42 | global $admin_Plugins; |
8 | 43 | |||
9 | 44 | global $dispatcher; | 43 | global $dispatcher; |
10 | 45 | 44 | ||
11 | 46 | $Table = new Table(); | 45 | $Table = new Table(); |
12 | 47 | |||
13 | 48 | $Table->title = T_('Plugins available for installation'); | 46 | $Table->title = T_('Plugins available for installation'); |
14 | 49 | |||
15 | 50 | $Table->global_icon( T_('Cancel install!'), 'close', regenerate_url(), T_('Cancel'), 3, 4 ); | 47 | $Table->global_icon( T_('Cancel install!'), 'close', regenerate_url(), T_('Cancel'), 3, 4 ); |
16 | 51 | |||
17 | 52 | $Table->cols = array( | 48 | $Table->cols = array( |
27 | 53 | array( 'th' => T_('Plugin') ), | 49 | array( 'th' => T_('Plugin') ), |
28 | 54 | array( 'th' => T_('Description') ), | 50 | array( 'th' => T_('Description') ), |
29 | 55 | array( 'th' => T_('Version') ), | 51 | array( 'th' => T_('Version') ), |
30 | 56 | array( 'th' => T_('Help'), | 52 | array( 'th' => T_('Help'), 'td_class' => 'nowrap' ), |
31 | 57 | 'td_class' => 'nowrap' ), | 53 | array( 'th' => T_('Actions'), 'td_class' => 'nowrap' ), |
32 | 58 | array( 'th' => T_('Actions'), | 54 | ); |
24 | 59 | 'td_class' => 'nowrap' ), | ||
25 | 60 | ); | ||
26 | 61 | |||
33 | 62 | $Table->display_init(); | 55 | $Table->display_init(); |
34 | 63 | |||
35 | 64 | $Table->display_list_start(); | 56 | $Table->display_list_start(); |
36 | 65 | 57 | ||
37 | 66 | // TITLE / COLUMN HEADERS: | 58 | // TITLE / COLUMN HEADERS: |
38 | @@ -85,91 +77,85 @@ | |||
39 | 85 | 77 | ||
40 | 86 | while( $loop_Plugin = & $AvailablePlugins->get_next() ) | 78 | while( $loop_Plugin = & $AvailablePlugins->get_next() ) |
41 | 87 | { | 79 | { |
42 | 80 | // first thing is to exclude any plugin that has reached it's $number_of_installs limit | ||
43 | 81 | list( $registrations, $seq_number ) = $admin_Plugins->count_regs( $loop_Plugin->classname, true ); | ||
44 | 82 | if( ! ( $current_User->check_perm( 'options', 'edit', false ) | ||
45 | 83 | && ( ! isset( $loop_Plugin->number_of_installs ) | ||
46 | 84 | || $registrations < $loop_Plugin->number_of_installs ) ) ) | ||
47 | 85 | { // plugin not available for installation by this user | ||
48 | 86 | continue; | ||
49 | 87 | } | ||
50 | 88 | 88 | ||
51 | 89 | // create a plugin group heading | ||
52 | 89 | if( $loop_Plugin->group !== $current_group && $number_of_groups ) | 90 | if( $loop_Plugin->group !== $current_group && $number_of_groups ) |
53 | 90 | { // Reason why $current_group is false | 91 | { // Reason why $current_group is false |
54 | 91 | $current_group = $loop_Plugin->group; | 92 | $current_group = $loop_Plugin->group; |
55 | 92 | ?> | 93 | ?> |
68 | 93 | <tr class="group"> | 94 | <tr class="group"><td colspan="5" class="first"> |
69 | 94 | <td colspan="5" class="first"><?php | 95 | <?php |
70 | 95 | if( $current_group == '' || $current_group == 'Un-Grouped' ) | 96 | if( $current_group == '' || $current_group == 'Un-Grouped' ) |
71 | 96 | { | 97 | { |
72 | 97 | echo T_('Un-Classified'); | 98 | echo T_('Un-Classified'); |
73 | 98 | } | 99 | } |
74 | 99 | else | 100 | else |
75 | 100 | { | 101 | { |
76 | 101 | echo $current_group; | 102 | echo $current_group; |
77 | 102 | } | 103 | } |
78 | 103 | ?></td> | 104 | ?> |
79 | 104 | </tr> | 105 | </td></tr> |
80 | 105 | <?php | 106 | <?php |
81 | 106 | } | 107 | } |
82 | 107 | 108 | ||
83 | 108 | $Table->display_line_start(); | 109 | $Table->display_line_start(); |
84 | 109 | 110 | ||
148 | 110 | $Table->display_col_start(); | 111 | // the "Plugin" column |
149 | 111 | ?> | 112 | $Table->display_col_start(); |
150 | 112 | <strong><a title="<?php echo T_('Display info') ?>" href="<?php echo regenerate_url( 'action,plugin_class', 'action=info&plugin_class='.$loop_Plugin->classname) ?>"> | 113 | ?> |
151 | 113 | <?php echo format_to_output($loop_Plugin->name); ?></a></strong> | 114 | <strong><a title="<?php echo T_('Display info') ?>" href="<?php echo regenerate_url( 'action,plugin_class', 'action=info&plugin_class='.$loop_Plugin->classname) ?>"> |
152 | 114 | <?php | 115 | <?php echo format_to_output($loop_Plugin->name); ?></a></strong> |
153 | 115 | $Table->display_col_end(); | 116 | <?php |
154 | 116 | 117 | $Table->display_col_end(); | |
155 | 117 | $Table->display_col_start(); | 118 | |
156 | 118 | echo format_to_output($loop_Plugin->short_desc); | 119 | // the "Description" column |
157 | 119 | /* | 120 | $Table->display_col_start(); |
158 | 120 | // Available events: | 121 | echo format_to_output($loop_Plugin->short_desc); |
159 | 121 | $registered_events = implode( ', ', $AvailablePlugins->get_registered_events( $loop_Plugin ) ); | 122 | $Table->display_col_end(); |
160 | 122 | if( empty($registered_events) ) | 123 | |
161 | 123 | { | 124 | // the "Version" column |
162 | 124 | $registered_events = '-'; | 125 | $Table->display_col_start(); |
163 | 125 | } | 126 | $clean_version = preg_replace( array('~^(CVS\s+)?\$'.'Revision:\s*~i', '~\s*\$$~'), '', $loop_Plugin->version ); |
164 | 126 | echo '<span class="advanced_info notes"><br />'.T_('Registered events:').' '.$registered_events.'</span>'; | 127 | echo format_to_output($clean_version); |
165 | 127 | */ | 128 | $Table->display_col_end(); |
166 | 128 | $Table->display_col_end(); | 129 | |
167 | 129 | 130 | // the "Help" column | |
168 | 130 | $Table->display_col_start(); | 131 | $Table->display_col_start(); |
169 | 131 | $clean_version = preg_replace( array('~^(CVS\s+)?\$'.'Revision:\s*~i', '~\s*\$$~'), '', $loop_Plugin->version ); | 132 | echo action_icon( T_('Display info'), 'info', regenerate_url( 'action,plugin_class', 'action=info&plugin_class='.$loop_Plugin->classname ) ); |
170 | 132 | 133 | // Help icons, if available: | |
171 | 133 | echo format_to_output($clean_version); | 134 | $help_icons = array(); |
172 | 134 | $Table->display_col_end(); | 135 | if( $help_external = $loop_Plugin->get_help_link() ) |
173 | 135 | 136 | { | |
174 | 136 | // HELP COL: | 137 | $help_icons[] = $help_external; |
175 | 137 | $Table->display_col_start(); | 138 | } |
176 | 138 | echo action_icon( T_('Display info'), 'info', regenerate_url( 'action,plugin_class', 'action=info&plugin_class='.$loop_Plugin->classname ) ); | 139 | if( $help_internal = $loop_Plugin->get_help_link('$readme') ) |
177 | 139 | // Help icons, if available: | 140 | { |
178 | 140 | $help_icons = array(); | 141 | $help_icons[] = $help_internal; |
179 | 141 | if( $help_external = $loop_Plugin->get_help_link() ) | 142 | } |
180 | 142 | { | 143 | if( ! empty($help_icons) ) |
181 | 143 | $help_icons[] = $help_external; | 144 | { |
182 | 144 | } | 145 | echo ' '.implode( ' ', $help_icons ); |
183 | 145 | if( $help_internal = $loop_Plugin->get_help_link('$readme') ) | 146 | } |
184 | 146 | { | 147 | $Table->display_col_end(); |
185 | 147 | $help_icons[] = $help_internal; | 148 | |
186 | 148 | } | 149 | // the "Actions" column |
187 | 149 | if( ! empty($help_icons) ) | 150 | $Table->display_col_start(); |
188 | 150 | { | 151 | echo '[<a href="'.$dispatcher.'?ctrl=plugins&action=install&plugin='.rawurlencode($loop_Plugin->classname).'">'. |
189 | 151 | echo ' '.implode( ' ', $help_icons ); | 152 | T_('Install'); |
190 | 152 | } | 153 | if( $seq_number > 1 ) |
191 | 153 | $Table->display_col_end(); | 154 | { // This plugin is already installed |
192 | 154 | 155 | echo ' #'.$seq_number; | |
193 | 155 | $Table->display_col_start(); | 156 | } |
194 | 156 | list( $registrations, $seq_number ) = $admin_Plugins->count_regs( $loop_Plugin->classname, true ); | 157 | echo '</a>]'; |
195 | 157 | 158 | $Table->display_col_end(); | |
133 | 158 | if( $current_User->check_perm( 'options', 'edit', false ) | ||
134 | 159 | && ( ! isset( $loop_Plugin->number_of_installs ) | ||
135 | 160 | || $registrations < $loop_Plugin->number_of_installs ) ) | ||
136 | 161 | { // number of installations are not limited or not reached yet and user has "edit options" perms | ||
137 | 162 | ?> | ||
138 | 163 | [<a href="<?php echo $dispatcher ?>?ctrl=plugins&action=install&plugin=<?php echo rawurlencode($loop_Plugin->classname) ?>"><?php | ||
139 | 164 | echo T_('Install'); | ||
140 | 165 | if( $seq_number > 1 ) | ||
141 | 166 | { // This plugin is already installed | ||
142 | 167 | echo ' #', $seq_number; | ||
143 | 168 | } | ||
144 | 169 | ?></a>] | ||
145 | 170 | <?php | ||
146 | 171 | } | ||
147 | 172 | $Table->display_col_end(); | ||
196 | 173 | 159 | ||
197 | 174 | $Table->display_line_end(); | 160 | $Table->display_line_end(); |
198 | 175 | 161 | ||
199 | @@ -181,21 +167,6 @@ | |||
200 | 181 | // BODY END: | 167 | // BODY END: |
201 | 182 | $Table->display_body_end(); | 168 | $Table->display_body_end(); |
202 | 183 | 169 | ||
203 | 184 | |||
204 | 185 | $Table->display_list_end(); | 170 | $Table->display_list_end(); |
205 | 186 | 171 | ||
206 | 187 | |||
207 | 188 | // Note about how to make plugins available for installation. | ||
208 | 189 | // It should make clear that the above list are not all available plugins (e.g. through an online channel)! | ||
209 | 190 | global $plugins_path, $app_homepage; | ||
210 | 191 | echo '<p>'; | ||
211 | 192 | echo T_('The above plugins are those already installed into your "plugins" directory.'); | ||
212 | 193 | echo "</p>\n<p>"; | ||
213 | 194 | printf( T_('You can find more plugins online at %s or other channels.'), '<a href="'.$app_homepage.'">'.$app_homepage.'</a>'); | ||
214 | 195 | echo "</p>\n<p>"; | ||
215 | 196 | printf( T_('To make a plugin available for installation, extract it into the "%s" directory on the server.'), | ||
216 | 197 | rel_path_to_base($plugins_path) ); | ||
217 | 198 | echo '</p>'; | ||
218 | 199 | |||
219 | 200 | |||
220 | 201 | ?> | ||
221 | 202 | \ No newline at end of file | 172 | \ No newline at end of file |
222 | 173 | ?> |
- In diff line 51, you removed the $current_sub_group assignment. This causes empty sub-group headers to appear. >display_ col_end( ), not closing the Description column (the browser works around this and renders the table correctly, but it's ugly code). Plugins- >count_ regs() in diff line 135 as that method is already being called in diff line 39 now.
- In diff line 105, you accidentally commented out a call to $Table-
- Looks like you broke the code which _can_ display the available events of a plugin (diff lines 101-104), but that code is commented out anyway, so that's not a real issue.
- It should be possible to remove the second call to $admin_
- You removed the note explaining where to upload new plugins (diff ll. 221). I'm not sure this is a good idea...