known issue [BUG 4.7.16] Incosistent implementation of core extension "EditorLocations::attachmentLookup()"

[teraßyte]

As per the title, the implementation of the extension's EditorLocations::attachmentLookup() method is inconsistent when you don't allow attachments. No matter how I implement it, or which exception I throw, it won't work for all locations.

 

1) \applications\core\extensions\core\EditorMedia\Attachment.php (lines 140-156):

• The code checks if the method exists before calling it.
• The code checks for 2 exceptions being thrown:
  • \LogicException
  • \BadMethodCallException

					if( method_exists( static::$loadedExtensions[$map['location_key']], 'attachmentLookup'))
					{
						try {
							$url = static::$loadedExtensions[$map['location_key']]->attachmentLookup($map['id1'], $map['id2'], $map['id3']);

							/* Test url() method to prevent BadMethodCallException from the template below - an attachment may be
								located within a Node class that doesn't support urls, such as CMS Blocks. */

							if ($url instanceof \IPS\Content or $url instanceof \IPS\Node\Model){
								$url->url();
							}

							static::$locations[$attachId][] = $url;
						} catch (\LogicException $e) {
						} catch (\BadMethodCallException $e) {
						}
					}

 

2) \applications\core\modules\admin\overview\files.php (lines 209-219):

• The code checks if the method exists before calling it.
• The code checks for a single exception being thrown:
  • \LogicException

			if ( isset( $loadedExtensions[ $map['location_key'] ] ) AND method_exists( $loadedExtensions[ $map['location_key'] ], 'attachmentLookup' ) )
			{
				try
				{
					if ( $url = $loadedExtensions[ $map['location_key'] ]->attachmentLookup( $map['id1'], $map['id2'], $map['id3'] ) )
					{
						$locations[] = $url;
					}
				}
				catch ( \LogicException $e ) { }
			}

 

3) \applications\core\modules\front\system\attachments.php (lines 87-100):

• The code doesn't check if the method exists before calling it.
• The code checks for a single exception being thrown:
  • \OutOfRangeException

		/* Check Permission */
		$exploded = explode( '_', $attachment['location_key'] );
		try
		{
			$extensions = \IPS\Application::load( $exploded[0] )->extensions( 'core', 'EditorLocations' );
			if ( isset( $extensions[ $exploded[1] ] ) )
			{
				$attachmentItem = $extensions[ $exploded[1] ]->attachmentLookup( $attachment[ 'id1' ], $attachment[ 'id2' ], $attachment[ 'id3' ] );
			}
		}
		catch ( \OutOfRangeException $e )
		{
			\IPS\Output::i()->json( array( 'error' => 'no_permission' ) );
		}

 

4) \system\Content\Statistics.php (lines 398-408):

• The code doesn't check if the method exists before calling it.
• The code checks for a single exception being thrown:
  • \LogicException
  • \BadMethodCallException

				if ( isset( static::$loadedExtensions[ $map['location_key'] ] ) )
				{
					try
					{
						$url = static::$loadedExtensions[ $map['location_key'] ]->attachmentLookup( $map['id1'], $map['id2'], $map['id3'] );

						$return[ $k ]['commentUrl'] = (string) $url->url();
					}
					catch ( \LogicException $e ) { }
					catch ( \BadMethodCallException $e ){ }
				}

 

===

To make a summary of the implementations:

1. All OK.
2. It should check for the \BadMethodCallException exception.
3. It should check if the method exists before calling it, and both \LogicException and \BadMethodCallException exceptions aren't being checked. Rather, it checks for a \OutOfRangeException exception which is never thrown according to the method's phpDoc.
4. It should check if the method exists before calling it.

 

I could throw a \LogicException, but as it is it would break implementation #3 anyway.

Edited March 21 by teraßyte

[Marc Stridgen]

Thank you for bringing this issue to our attention! I can confirm this should be further reviewed and I have logged an internal bug report for our development team to investigate and address as necessary, in a future maintenance release.