r69650 MediaWiki - Code Review archive

require_once('/var/www/sites/all/modules/queue2civicrm/Stomp.php'); // why are these Stomps different?!

Don't hard-code absolute paths. Defer the load of Stomp.php until setup is complete, and then use a regular configuration variable to find it. For instance, you could put the require_once() in StompPFPPendingProcessor::__construct(). Unlike in compiled languages, as long as the class is defined before the code that uses it is actually run, it won't cause an error.

define( 'LOG_LEVEL_QUIET', 0 ); // disables all logging
define( 'LOG_LEVEL_INFO', 1 ); // some useful logging information
define( 'LOG_LEVEL_DEBUG', 2 ); // verbose logging for debug

Please use class constants instead of global constants, or prefix the names with the name of the extension, to avoid conflicts with other extensions that have logging.

			// empty pending_transaction
			if ( isset( $message )) unset( $message );

This code is unnecessary and should be removed.

			// the message is in it's raw format, we need to decode just it's body

Possessive "its" has no apostrophe.

		// we only really care about the 'RESULT' portion of the status message
		$result = strstr( $status, 'RESULT' );

Indeed, that is all we really care about. So parse the response properly so that if the word RESULT appears in some value part before the actual result key, your code will still return that data about which we care so much. For example, if I gave this function the string:

XRESULT=x&YRESULT=y&BATCHID=NORESULT&RESULT=3

The result code should be 3, not x, y or an empty string. You should test the function separately using maintenance/eval.php.

		// loop through our options and set their values, 
		// overrides local settings with command line options
		foreach ( $options as $mw_option => $m_arg_name ) {
			global ${$mw_option};

			// replace - with _ from CL args to map to class properties
			$property = str_replace( "-", "_", $m_arg_name );

I think you're trying to be too clever here, to the detriment of readability. This style of code makes it difficult to search the code for a given member variable (e.g. $this->activemq_stomp_uri), and to find out where that member variable came from. If you added var declarations to the class, then you'd have somewhere to document what they all mean. And if you made load_config_options() look more like this:

		$this->pfp_url = $this->getOption( 'pfp-url', $wgPayflowProURL );
		$this->pfp_partner_id = $this->getOption( 'pfp-partner-id', $wgPayflowProPartnerID );
		$this->pfp_vendor_id = $this->getOption( 'pfp-vendor-id', $wgPayflowProVendorID );
		...

then the data flow would be immediately obvious, the code would be searchable, and static analysis would work on it.

			"The level of logging you would like to enable.  Options:\n\t0 - No output\n\t1 - Normal, minimal output\n\t2 - Debug, verbose output", 

So you have readable names for these options in the code, but not for the users? Don't users need readable options even more than developers? It's not hard to write something like:

		$logLevels = array( 
			'none' => self::NONE, 
			'info' => self::INFO, 
			'debug' => self::DEBUG );
		if ( isset( $logLevels[$level] ) ) {
			$this->log_level = $logLevels[$level];
		}

The files in question have been removed as of r71653. This provided rather custom functionality, which is now handled by scripts outside of MW.