Fetch media by tag.

Thanks for the patch, I will do some testing and give feedback as soon as I find some time.

Cool it looks great!

I've made some adjustments to the exception message and added a watchdog.
Other than that, from my point of view, this is good to go in.

instagram_block does exactly what I want, but needs a "tag" field :)

Do you need me to release a new version, or are you happy with this just being on 7.x-1.x for a while?

1. You __construct() method should not return $this.

2. +++ b/instagram_block.lib.php
   @@ -12,13 +12,44 @@ class InstagramRequest {
   +    $values = $this->values;
   

   I don't think you need to shorten this everywhere, makes more sense to me as longer.

3. +++ b/instagram_block.module
   @@ -126,7 +133,19 @@ function instagram_block_block_view($delta = '') {
   +        }
   ...
   +      } ¶
   

   Whitespace error.

4. +      catch (\Exception $e) {
   +        drupal_set_message(t('Instagram Block: There was an error connecting to instagram. See log messages for details'), 'error');
   +        watchdog('http', 'Instagram Block: @error', array('@error' => $e->getMessage()));
   +      }
   

   I think here you don't need to worry about drupal_set_message and watchdog - that should all be handled by Drupal's existing error handling. I suggest replacing the two lines in this catch with:

    watchdog_exception('instagram_block', $e);
   throw $e; 

   I also don't think you need the \ before Exception as there is no namespacing in Drupal 7 anyway.

Great, thanks @rlmumford.

So I have implemented most of your changes. I wasn't sure about your 'throw $e;' because passing the exception back to drupal's error handling will actually stop the page from loading, and I didn't think that a block not getting the correct response should bring the site down. But then I thought of the different reasons why this exception would happen, and the only reasons I could think of involved developers messing with code (as we don't allow front end users to access the url).

So I've attached a screenshot of the error handling for that exception and I'm happy with it.

The configuration for the block presents the same configuration fields as the main configuration page. Is there any specific intention to this @yanniboi?

No I've been thinking about that for a while. I think that there should be module specific settings and block specific settings. However because there was always just one block I didn't worry much about separating them. I also think that a block type with unlimited instances (eg like custom blocks, menu blocks, twitter blocks, etc.) is beyond the scope of this module.

I think that providing two blocks, one for user related, one for tag related, would work. However if we end up having more than two we might need to rethink that approach.

In short I think you are right. Lets add another block in this issue, and start another issue for fixing the settings.

Been away this weekend, but having a look at this tonight.

First thoughts include:

1. +++ b/instagram_block.module
   @@ -11,8 +11,13 @@
    function instagram_block_block_info() {
   -  $blocks['instagram_block'] = array(
   -    'info' => t('Instagram Block'),
   +  $blocks['instagram_user_block'] = array(
   +    'info' => t('Instagram Personal Block'),
   +    'cache' => DRUPAL_CACHE_GLOBAL,
   +  );
   +
   +  $blocks['instagram_tag_block'] = array(
   +    'info' => t('Instagram Tag Block'),
        'cache' => DRUPAL_CACHE_GLOBAL,
      );
   

   Changing the original block's name breaks backward compatibility.

2. +++ b/instagram_block.module
   @@ -26,68 +31,95 @@ function instagram_block_block_info() {
   +      $description = t('Configurations in this block overrides the !globalconfig.', array(
   +        '!globalconfig' => l('global configuration', 'admin/config/content/instagram_block'),
   +      ));
   

   Not entirely happy with the way config works in the light of having multiple blocks.

I think I am going to add a blocker issue. If we are going to have multiple blocks then the config really needs sorting out. I will open another issue to fix config, get that patch in first and then rebase the work we are doing here.

Hopefully I'll have made some good progress by the end of the night.

Updated issue summary

I have rebased your last patch on top of the first patch on #2197905: Block config and module config should be stored separately. I only updated it with respect to necessary changes in light of the new commit it is applied on top.

The next patch will be my changes on top of your changes and I think at this point an interdiff will be due.

And here is my patch! Wow, long night!

I have renamed the block deltas to maintain backward compatibility, I have refactored the block config forms (not really that tidy, but it works) and I now pass $values and $config to the InstagramRequest constructor as they are stored in separate values.

Have a look and see what you think...

While reviewing this patch I found some errors in the config page links...

I've reviewed this patch and am committing it.