Merge lp:~edb/quam-plures/plugins_list_available into lp:quam-plures

[EdB]

http://forums.quamplures.net/viewtopic.php?f=6&t=900

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.

[Tilman Blumenbach (tblue)]

- In diff line 51, you removed the $current_sub_group assignment. This causes empty sub-group headers to appear.
- In diff line 105, you accidentally commented out a call to $Table->display_col_end(), not closing the Description column (the browser works around this and renders the table correctly, but it's ugly code).
- 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_Plugins->count_regs() in diff line 135 as that method is already being called in diff line 39 now.
- You removed the note explaining where to upload new plugins (diff ll. 221). I'm not sure this is a good idea...

review: Needs Fixing

[EdB (edb)]

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!

[lp:~edb/quam-plures/plugins_list_available]

7620. By EdB on 2011-09-01

core to 7620

[EdB (edb)]

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?

review: Needs Fixing

[Tilman Blumenbach (tblue)]

> 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.

[lp:~edb/quam-plures/plugins_list_available]

7621. By EdB on 2011-09-01

fixed broken table issue, fixed broken and commented out code

7622. By EdB on 2011-09-01

core to 7624

[EdB (edb)]

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.

[lp:~edb/quam-plures/plugins_list_available]

7623. By EdB on 2011-09-04

core to 7628

7624. By EdB on 2011-09-05

got rid of second "something->count( something else )" line that tblue pointed out wasn't needed

[EdB (edb)]

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.

[EdB (edb)]

oops

review: Needs Resubmitting

[Tilman Blumenbach (tblue)]

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=plugins&amp;action=install&amp;plugin=<?php echo rawurlencode($loop_Plugin->classname) ?>"><?php

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.

review: Needs Fixing

[EdB (edb)]

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.

[lp:~edb/quam-plures/plugins_list_available]

7625. By EdB on 2011-09-06

fix stray space between [ and the word Install

[Tilman Blumenbach (tblue)]

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.

review: Approve

[lp:~edb/quam-plures/plugins_list_available]

7626. By EdB on 2011-09-06

removing redundant permissions check

[EdB (edb)]

done

[Tilman Blumenbach (tblue)]

Well, all good then!

review: Approve

[Tilman Blumenbach (tblue)]

...nope.

Parse error: syntax error, unexpected T_BOOLEAN_OR, expecting ',' or ')' in /home/tilman/public_html/quamplures/trunk/qp_inc/plugins/views/_plugin_list_available.view.php on line 171

It's a incorrectly placed brace.

review: Needs Fixing

[Tilman Blumenbach (tblue)]

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.

[lp:~edb/quam-plures/plugins_list_available]

7627. By EdB on 2011-09-07

fixed flawed code by removing redundant check, removed commented out code because it has no value

[EdB (edb)]

HOPEFULLY this is the last time it gets fixed

review: Needs Resubmitting

[Tilman Blumenbach (tblue)]

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='.$dispatcher.'?ctrl=plugins&amp;action=install&amp;plugin='.rawurlencode($loop_Plugin->classname).'">'.

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_admin::install() because the double quote should be escaped and not cause an SQL error! Anyway, not in the scope of this branch. Just fix the missing double quote and in the meanwhile, I will report a new bug.

Once you have fixed the missing quote, everything should FINALLY be okay. Sigh.

review: Needs Fixing

[lp:~edb/quam-plures/plugins_list_available]

7628. By EdB on 2011-09-07

fuckit

[EdB (edb)]

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.

review: Needs Resubmitting

[Tilman Blumenbach (tblue)]

All good now.

review: Approve

[Tilman Blumenbach (tblue)]

Does somebody else want to review this one? I usually merge branches after they have been approved by at least two reviewers.

[lp:~edb/quam-plures/plugins_list_available]

7629. By EdB on 2011-09-13

matches core 7632 after conflict resolution

[Lee Turner (leeturner)]

This one is next on my list

=== modified file 'qp_inc/plugins/views/_plugin_list_available.view.php'
--- qp_inc/plugins/views/_plugin_list_available.view.php	2011-09-04 22:27:27 +0000
+++ qp_inc/plugins/views/_plugin_list_available.view.php	2011-09-13 21:46:49 +0000
@@ -40,27 +40,19 @@
  * @var Plugins
  */
 global $admin_Plugins;
-
 global $dispatcher;
 
 $Table = new Table();
-
 $Table->title = T_('Plugins available for installation');
-
 $Table->global_icon( T_('Cancel install!'), 'close', regenerate_url(), T_('Cancel'), 3, 4 );
-
 $Table->cols = array(
-		array( 'th' => T_('Plugin') ),
-		array( 'th' => T_('Description') ),
-		array( 'th' => T_('Version') ),
-		array( 'th' => T_('Help'),
-					 'td_class' => 'nowrap' ),
-		array( 'th' => T_('Actions'),
-					 'td_class' => 'nowrap' ),
-	);
-
+	array( 'th' => T_('Plugin') ),
+	array( 'th' => T_('Description') ),
+	array( 'th' => T_('Version') ),
+	array( 'th' => T_('Help'), 'td_class' => 'nowrap' ),
+	array( 'th' => T_('Actions'), 'td_class' => 'nowrap' ),
+);
 $Table->display_init();
-
 $Table->display_list_start();
 
 // TITLE / COLUMN HEADERS:
@@ -85,91 +77,85 @@
 
 while( $loop_Plugin = & $AvailablePlugins->get_next() )
 {
+	// first thing is to exclude any plugin that has reached it's $number_of_installs limit
+	list( $registrations, $seq_number ) = $admin_Plugins->count_regs( $loop_Plugin->classname, true );
+	if( ! ( $current_User->check_perm( 'options', 'edit', false )
+		&& ( ! isset( $loop_Plugin->number_of_installs )
+		|| $registrations < $loop_Plugin->number_of_installs ) ) )
+	{ // plugin not available for installation by this user
+		continue;
+	}
 
+	// create a plugin group heading
 	if( $loop_Plugin->group !== $current_group && $number_of_groups )
 	{ // Reason why $current_group is false
 		$current_group = $loop_Plugin->group;
 		?>
-		<tr class="group">
-			<td colspan="5" class="first"><?php
-			if( $current_group == '' || $current_group == 'Un-Grouped' )
-			{
-				echo T_('Un-Classified');
-			}
-			else
-			{
-				echo $current_group;
-			}
-			?></td>
-		</tr>
+		<tr class="group"><td colspan="5" class="first">
+		<?php
+		if( $current_group == '' || $current_group == 'Un-Grouped' )
+		{
+			echo T_('Un-Classified');
+		}
+		else
+		{
+			echo $current_group;
+		}
+		?>
+		</td></tr>
 		<?php
 	}
 
 	$Table->display_line_start();
 
-		$Table->display_col_start();
-	  ?>
-			<strong><a title="<?php echo T_('Display info') ?>" href="<?php echo regenerate_url( 'action,plugin_class', 'action=info&amp;plugin_class='.$loop_Plugin->classname) ?>">
-	    <?php echo format_to_output($loop_Plugin->name); ?></a></strong>
-		<?php
-		$Table->display_col_end();
-
-		$Table->display_col_start();
-			echo format_to_output($loop_Plugin->short_desc);
-			/*
-			// Available events:
-			$registered_events = implode( ', ', $AvailablePlugins->get_registered_events( $loop_Plugin ) );
-			if( empty($registered_events) )
-			{
-				$registered_events = '-';
-			}
-			echo '<span class="advanced_info notes"><br />'.T_('Registered events:').' '.$registered_events.'</span>';
-			*/
-		$Table->display_col_end();
-
-		$Table->display_col_start();
-			$clean_version = preg_replace( array('~^(CVS\s+)?\$'.'Revision:\s*~i', '~\s*\$$~'), '', $loop_Plugin->version );
-
-			echo format_to_output($clean_version);
-		$Table->display_col_end();
-
-		// HELP COL:
-		$Table->display_col_start();
-			echo action_icon( T_('Display info'), 'info', regenerate_url( 'action,plugin_class', 'action=info&amp;plugin_class='.$loop_Plugin->classname ) );
-			// Help icons, if available:
-			$help_icons = array();
-			if( $help_external = $loop_Plugin->get_help_link() )
-			{
-				$help_icons[] = $help_external;
-			}
-			if( $help_internal = $loop_Plugin->get_help_link('$readme') )
-			{
-				$help_icons[] = $help_internal;
-			}
-			if( ! empty($help_icons) )
-			{
-				echo ' '.implode( ' ', $help_icons );
-			}
-		$Table->display_col_end();
-
-		$Table->display_col_start();
-			list( $registrations, $seq_number ) = $admin_Plugins->count_regs( $loop_Plugin->classname, true );
-
-			if( $current_User->check_perm( 'options', 'edit', false )
-					&& ( ! isset( $loop_Plugin->number_of_installs )
-					     || $registrations < $loop_Plugin->number_of_installs ) )
-			{ // number of installations are not limited or not reached yet and user has "edit options" perms
-				?>
-				[<a href="<?php echo $dispatcher ?>?ctrl=plugins&amp;action=install&amp;plugin=<?php echo rawurlencode($loop_Plugin->classname) ?>"><?php
-					echo T_('Install');
-					if( $seq_number > 1 )
-					{	// This plugin is already installed
-						echo ' #', $seq_number;
-					}
-					?></a>]
-				<?php
-			}
-		$Table->display_col_end();
+	// the "Plugin" column
+	$Table->display_col_start();
+	?>
+	<strong><a title="<?php echo T_('Display info') ?>" href="<?php echo regenerate_url( 'action,plugin_class', 'action=info&amp;plugin_class='.$loop_Plugin->classname) ?>">
+	<?php echo format_to_output($loop_Plugin->name); ?></a></strong>
+	<?php
+	$Table->display_col_end();
+
+	// the "Description" column
+	$Table->display_col_start();
+	echo format_to_output($loop_Plugin->short_desc);
+	$Table->display_col_end();
+
+	// the "Version" column
+	$Table->display_col_start();
+	$clean_version = preg_replace( array('~^(CVS\s+)?\$'.'Revision:\s*~i', '~\s*\$$~'), '', $loop_Plugin->version );
+	echo format_to_output($clean_version);
+	$Table->display_col_end();
+
+	// the "Help" column
+	$Table->display_col_start();
+	echo action_icon( T_('Display info'), 'info', regenerate_url( 'action,plugin_class', 'action=info&amp;plugin_class='.$loop_Plugin->classname ) );
+	// Help icons, if available:
+	$help_icons = array();
+	if( $help_external = $loop_Plugin->get_help_link() )
+	{
+		$help_icons[] = $help_external;
+	}
+	if( $help_internal = $loop_Plugin->get_help_link('$readme') )
+	{
+		$help_icons[] = $help_internal;
+	}
+	if( ! empty($help_icons) )
+	{
+		echo ' '.implode( ' ', $help_icons );
+	}
+	$Table->display_col_end();
+
+	// the "Actions" column
+	$Table->display_col_start();
+	echo '[<a href="'.$dispatcher.'?ctrl=plugins&amp;action=install&amp;plugin='.rawurlencode($loop_Plugin->classname).'">'.
+		T_('Install');
+	if( $seq_number > 1 )
+	{ // This plugin is already installed
+		echo ' #'.$seq_number;
+	}
+	echo '</a>]';
+	$Table->display_col_end();
 
 	$Table->display_line_end();
 
@@ -181,21 +167,6 @@
 // BODY END:
 $Table->display_body_end();
 
-
 $Table->display_list_end();
 
-
-// Note about how to make plugins available for installation.
-// It should make clear that the above list are not all available plugins (e.g. through an online channel)!
-global $plugins_path, $app_homepage;
-echo '<p>';
-echo T_('The above plugins are those already installed into your "plugins" directory.');
-echo "</p>\n<p>";
-printf( T_('You can find more plugins online at %s or other channels.'), '<a href="'.$app_homepage.'">'.$app_homepage.'</a>');
-echo "</p>\n<p>";
-printf( T_('To make a plugin available for installation, extract it into the "%s" directory on the server.'),
-	rel_path_to_base($plugins_path) );
-echo '</p>';
-
-
-?>
\ No newline at end of file
+?>