Add an option to pass through custom scald form (hook_scald_add_form)

Scald Gallery is also a good example for atom providers without the add form step.

We get this request often to skip the first step.

I think having the first step is mainly useful for plupload situations when it can be used as a global prefil for all atoms.

However for most providers that do not have any file uploads its likely an unnecessary extra step and an extra button click required from the user.
Of course submitting the first step will not result in an atom creation and is mainly handled by the form api, while the second step uses fields on the entity and will create a new entity on submit. So the second step can be considered as a review step.
However the issue is that by default in order to have the same fields on the first and second step, one needs to have the same fields available in both the form API and in the field system, which is not always that easy. And sometimes its not implemented so the first step is actually empty (except maybe the title field is there).
But even if its implemented, sometimes it just feels unnecessary to have the first step there.

The question is whether this should be something the provider implements and then is forced on all users, or something that the site builder should be able to choose.
In some cases the provider might decide not to fully implement the first step and then the provider could choose to not have the first step, and then letting the site builder do it in a UI setting makes no sense.
But maybe in some cases the provider could give this option to the site builder.

So in a first round maybe the best is if we let the provider decide in code whether the first step should be skipped or not.

This is the first patch.

Basically if there is only one provider for the type and the provider module does not implement the "scald_add_form" hook then the add step is skipped.

Its not that easy though as the "scald_atom_add_form_add_submit" function has to run to set up how many atoms are needed and such. So later in the code where the scald $form_state is reconstructed when its empty (cause the "source" step was skipped) I called it specifically if the current step is "options" at that point.
I guess most of the code from the submit callback could be separated out to another function to not need to call a callback directly, but most of the function is required, as this way even though the form is not implemented by the provider and it is skipped, the fill and count hooks could still be useful in some situation.
Like a provider could still define that he would like 3 atoms created, or that the atoms should be prefilled in a specific way before the options form.

Now with this patch if the provider does not implement the form hook, the add step wont appear at all, it will start right at the options step.

There is one use case which this patch does not cover. Is that when you have more than one provider for a type. Because then the form should go to the source step, and then afterward it would need to decide whether to show the add step or not and skip like that.
Still need to figure out a solution for that. But for now the original issue would be solved with this patch.

And here is the updated patch which should solve the issue also when there is a source step involved.

Applied this patch, and tested with two use cases :
- The Scald: Gallery module, with add_form() and add_form_fill() commented out
- A minimal Image provider, with no add_form() or add_form_fill(), but a _scald_atom_providers_alter() which unsets all other providers in case "$_GET['q'] == 'atom/add/image/ajax'" (for "Quick and easy" single image upload in node/*/edit context)

Works nicely in both cases,
adopted ;)

I' currently considering a case where the provider might decide to define the "options" step as the default starting step, but a developer from custom code would like to add some elements to the add step which makes it necessary again.

In that case this patch would prevent that, as the add form will never appear to be altered by form alter.

Currently the idea is that we could extend the parameters in hook_scald_atom_providers to make it possible to include any parameter not only label.

And then to have the scald_atom_providers function which returns the labels as before (for backward compatibility), and to have another function scald_atom_providers_data lets say, which returns the additional data.
Of course a static cache can be included there.

Then lets say in scald_gallery we can add into that hook an extra parameter like default_starting_step => 'options', and then it would skip the add step. But it would be backwards compatible as the scald_atom_providers would work with the old array as well, and since the form hooks remain intact, they would work with an old scald core as well.
And also since we already have an alter in that function, a custom module can alter this parameter to 'add' to get back the add step and then to use form alter on it.

Example: maybe a custom import step is added in the first page, which pulls data from an internal system or something specific to the site, and then that can populate the options step from custom code as well.

I made a few changes on the concept to make the things more compact. Right now, the changes in .module file pass the tests and work fine, the stuff in scald.pages.inc needs work.

All the tests pass.
The only remaining issue that needs consideration is that we slightly break backwards compatibility. If a module implements hook_scald_atom_providers_alter(&$providers) to change a label, that hook needs an update after this patch.

Will do some more checks on this before committing.

IMHO, #4 was a simple and reliable patch

I think that

a developer from custom code would like to add some elements to the add step

should just re-introduce the add form in his custom code, which is a simple copy/paste

Conversely, after applying #10, I had to rework a two-years old scald_image_custom_scald_atom_providers_alter(), which was not that easy...

I would happily revert to #4 !

@gifad
Though the patch in #10 does introduce some other possibilities as well. Like a provider being able to define some default config in the hook that can be used later in the code.

Can you provide more details about that alter you have implemented? It would help me see the potential issue.

Also how can i readd that form in my custom code? With patch #4 the step would be totally skipped... by altering the ctools form definition somehow?

Also an issue we had with #4 is that if you apply that and start deleting the hook from the modules, then any module where you delete the hook from would need to require the latest scald dev, as otherwise its not backward compatible.
If you use an older scald with a newer provider that had this hook deleted would result in an empty add step with a confusing message.

10# fixes that by not removing the hooks, and whether the core implements skipping or not is entirely up to the core version.

with patch #4, tested scald_gallery.module with :

+$info = drupal_parse_info_file(drupal_get_path('module', 'scald') . '/scald.info');
+if (isset($info['project'])) {
+  $project = $info['project'];
+  $version = $info['version'];
+  dsm($project.' v'.$version);
+  if ($version < '7.x-1.3+12-dev') { 
 /**
  * Implements hook_scald_add_form().
  */
 function scald_gallery_scald_add_form(&$form, &$form_state) {
   $form['title'] = array(
     '#type' => 'textfield',
     '#title' => t('Gallery title'),
     '#required' => TRUE,
   );
 }
 
 /**
  * Implements hook_scald_add_form_fill().
  */
 function scald_gallery_scald_add_form_fill($atom, $form, $form_state) {
   $atom->title = $form_state['values']['title'];
 }
+  }
+}

(part of code stollen from admin_menu.inc)

does not work with current git version, of course, but will do if #4 committed...

[edit]
the developer from custom code would like to add some elements to the add step should combine this with if (!function_exists('scald_gallery_scald_add_form'))...

Actually #10 does lot more than just the ability to skip a step. It adds metadata to the provider. Any kind of added metadata would be supported in the future. Indeed, a bit more complex patch, but in the long run, it would add more flexibility. I guess the key debate here is if we will need this flexibility for other purposes or not. I'd say we do.

@gifad

Could you tell me in what situation have you used "scald_image_custom_scald_atom_providers_alter()"?

Are there other uses apart from renaming providers?

@nagy.bálint : the fatal operation was about renaming, but I also do set/unset/sort operations on the providers array;
This is not critical since I was just testing a patch, but it could be for a site admin who just want to upgrade to the latest dev release, because the site already moved to the "embed_div" storage method.
The base point is : drupal_alter issued by scald core are barely documented, even in scald.api.php; is that a case for a "change notice" ?

@gifad :
Yes patch #10 would require a change notice.

Unfortunately both patches have a backward compatibility issue, when you want to use a newer provider with an older scald core :(

So in any provider you update you would need to specify a dependency on the latest scald core which is kind of a problem.

Or well actually with patch #10 you could check for the version of scald in the hook implementation in the provider, and return a different result based on that, making it backward compatible.

The patch in #4 will never be backward compatible since we are removing hooks.

Hi all,

First, there is a bug in #10 : 'starting_step' is always taken from the first provider of providers list, so it works more or less randomly, depending of the number and order of list...
Here is a fixed scald_atom_add_form_source_submit() function (code need cleanup, but at least this shows problem and solution) :

/**
 * Handles the source step form submission.
 */
function scald_atom_add_form_source_submit(&$form, &$form_state) {
//dsm($form_state);
  $form_state['scald']['source'] = $form_state['values']['source'];
//dsm($form_state['scald']['source']); // correct
//dsm($form_state['scald']['provider']); // wrong (always 1st of list)
  $type = $form_state['scald']['type']->type;
  $module = $form_state['scald']['source'];
  $providers = scald_atom_providers('full');
  $provider = $providers[$type][$module];
  $form_state['scald']['provider'] = $provider;
//dsm($form_state['scald']['provider']); // better
  if (isset($form_state['scald']['provider']['starting_step']) && $form_state['scald']['provider']['starting_step'] == 'options') {
    scald_atom_add_form_add_submit($form, $form_state);
    $form_state['clicked_button']['#next'] = 'options';
  }
}

Second, about compatibilty;
In the ancient times, when I was a (object oriented) programmer, there was a rule : never change objects behaviors, just add new ones;
So I introduce a new hook : hook_scald_atom_providers_opt(), and the corresponding drupal_alter(), to be used by new providers using new options.
This should solve any compatibility problem I could think of, except the sequence of drupal_alters is not exactly preserved;

function scald_atom_providers($verbosity = 'basic', $reset = FALSE) {
  $types = &drupal_static(__FUNCTION__, NULL, $reset);
  if (!isset($types)) {
    $types = array();
    // collect old providers
    $hook = 'scald_atom_providers';
    $label_only = array();
    foreach (module_implements($hook) as $module) {
      foreach (module_invoke($module, $hook) as $type => $item) {
        $label_only[$type][$module] = $item;
      }
    }
    // collect new providers
    $hook = 'scald_atom_providers_opt';
    $array_only = array();
    foreach (module_implements($hook) as $module) {
      foreach (module_invoke($module, $hook) as $type => $item) {
        $array_only[$type][$module] = $item;
      }
    }
    // merge arrays in old format
    foreach ($array_only as $type => $item) {
      foreach ($item as $module => $data) {
        $label_only[$type][$module] = $data['label'];
      }
    }
    // invoke old alters    
    $hook = 'scald_atom_providers';
    drupal_alter($hook, $label_only);
    // convert to new format, eventually reintroduce options
    foreach($label_only as $type => $item) {
      foreach ($item as $module => $data) {
        $types[$type][$module] = array('label' => $data);
        if (!empty($array_only[$type][$module])) {
          foreach($array_only[$type][$module] as $name => $option) {
            if ($name != 'label') {
              $types[$type][$module][$name] = $option;
            }
          }
        }
      }
    }
    // invoke new alters    
    $hook = 'scald_atom_providers_opt';
    drupal_alter($hook, $types);
  }
  if ($verbosity == 'basic') {
    $label_only = array();
    foreach ($types as $type => $item) {
      foreach ($item as $module => $data) {
        $label_only[$type][$module] = $data['label'];
      }
    }
    return $label_only;
  }
  return $types;
}

Tested with scald_gallery :

/**
 * Implements hook_scald_atom_providers().
 */
function scald_gallery_scald_atom_providers() {
  return array(
    'gallery' => 'Gallery items upload'
  );
  // This code will never be hit, but is necessary to mark the string
  // for translation on localize.d.o
  t('Gallery items upload');
}

function scald_gallery_scald_atom_providers_opt() {
  return array(
    'gallery' => array(
      'label' => 'Gallery',
      'starting_step' => 'options',
    ),
  );
  // This code will never be hit, but is necessary to mark the string
  // for translation on localize.d.o
  t('Gallery items upload');
}

Sorry for such an ugly post, will make a patch later..
Have a nice week-end, anyway ;-)

Here is a patch for a complex, but I think reliable, implementation;
This issue should be fixed (with this patch or another) in relation with [meta] Path to stable release 1.4

Sorry, forgot half of the diffs...
[Edit] In fact, no, I was confusing git folder and scald-7.x-1.3+17-dev

Thanks for the patch.

I think we can make some simplification on the patch.

I have the following remarks:
1) The $provider variable is added to the form_state in scald_atom_add_page, and therefore the following code is not needed:

+ $type = $form_state['scald']['type']->type;
+ $module = $form_state['scald']['source'];
+ $providers = scald_atom_providers('full');
+ $provider = $providers[$type][$module];
+ $form_state['scald']['provider'] = $provider;

in scald_atom_add_form_source_submit

2) I think that we cannot ever remove the old hook that defines the provider and its label. And so the code would be a lot simpler if we did not do so much conversion, and the opts would define everything else except the label. So that way a provider can decide to implement only the base hook, and define the label which is mandatory. And then optionally can implement opts and define more parameters.
And in that case some of those conversions in the code are not needed anymore.
And so we can have two separate functions, where the old function to query the labels would remain the same, and the new function would only give back the rest of the data. And the alter is already separate in the code.
That would still keep backward compatibility, would have the options we need in this issue, and make the code look nicer and readable :)
I think originally the idea behind conversion was that the providers would switch to the new format entirely and not use the old at all, but in the new conception the provider would implement both hooks anyways. Then its better to simplify.

1) The $provider variable is added to the form_state in scald_atom_add_page, and therefore the following code is not needed:

Hey, nagy.balint, I understand my post #21 was a pain somewhere to read, but I explained that the provided provider was wrong;
I'll just repost my source code, before comments clean up for the patch:

/**
 * Handles the source step form submission.
 */
function scald_atom_add_form_source_submit(&$form, &$form_state) {
//dsm($form_state);
  $form_state['scald']['source'] = $form_state['values']['source'];
//dsm($form_state['scald']['source']); // correct
//dsm($form_state['scald']['provider']); // wrong (always 1st of list)
  $type = $form_state['scald']['type']->type;
  $module = $form_state['scald']['source'];
  $providers = scald_atom_providers('full');
  $provider = $providers[$type][$module];
  $form_state['scald']['provider'] = $provider;
//dsm($form_state['scald']['provider']); // better
  if (isset($form_state['scald']['provider']['starting_step']) && $form_state['scald']['provider']['starting_step'] == 'options') {
    scald_atom_add_form_add_submit($form, $form_state);
    $form_state['clicked_button']['#next'] = 'options';
  }
}

2) I think that we cannot ever remove the old hook

The old hook invocation is not removed : the code is shifted right one level of indentation, so diff shows a '-' at line 99 (of the patch), but it's reinserted line 109.

And about the code complexity, it's just the matrix old/new scald core vs old/new custom provider
My test case was an old provider doing label alterations, plus items additions/suppressions, and reordering, and a new provider doing about the same crunch.

Of course you can probably make the code look simpler (nicer ?), but you will perhaps break some sites associating various custom providers...
Not a problem for me personally, as I control all the code parts, but...

Oh, and did I say the provider was wrong in patch #10 ?

1) I will check that, but for me the original patch worked fine when i tested.

2) I know that. I said that because we cannot remove the old hook, there is no need to convert so much, so at least quarter of the code can be deleted, and maybe even better to separate it into 2 functions instead of having the verbosity, if we are going to have 2 separate hooks and alters anyways.
Please read my comment again.

the original patch worked fine when i tested.

You need two providers to make evident the bug in #10 :
The first with an add form, the second without;
If you select the second, you get he infamous message 'does it make sense ?'
Because the form state ['provider'] always points to the first one.

Please read my comment again.

Thanks,

You are right about 1)

About 2) to illustrate my point, here is the patch that does what i wrote in comment #26

I tested this, and since the original providers function remains intact, it wont have any backward compatibility problem.

Also the opt hook should only be called for providers that are registered with the base hook.
And the opt function will at the same time insert the label to the options array, as the provider does not have to specify the label again in the opt hook (that's why i wrote that since we cannot get rid of the original hook, we dont need to define the label in the opt hook)

Seems to work for me.

An example definition for scald_gallery that works:

/**
 * Implements hook_scald_atom_providers().
 */
function scald_gallery_scald_atom_providers() {
  return array(
    'gallery' => 'Gallery items upload',
  );
  // This code will never be hit, but is necessary to mark the string
  // for translation on localize.d.o
  t('Gallery items upload');
}

/**
 * Implements hook_scald_atom_providers_opt().
 */
function scald_gallery_scald_atom_providers_opt() {
  return array(
    'gallery' => array(
      'starting_step' => 'options',
    ),
  );
}

Good thing we have tests :)

I had no trouble to adapt my code to #32, and it works (for my use case).

It would not work with scald 1.3, but, fortunately, I have no backward compatibilty problem...

It would not work with scald 1.3

How exactly it would not work? with scald 1.3 you have only the base hook which has not changed, so all previous providers can leave it intact, and if they implement the new hook, and nobody to process it, it is also not an issue.
So please specify where it would not work.

It would not work in scald 1.3 just because my custom process is now done in mymodule_scald_atom_providers_opt_alter().
Making it work with scald 1.3 would be yet more work than #24, and lead to more complicated and/or with redundancy code I want to avoid.
More importantly, my custom providers require scald 1.3+dev, so it would not make sense.
(I did #24 for others...)

Im afraid i don't understand.

This is a new feature, so the new alter or new hook in a provider will not work with 1.3 but thats totally fine since this is a new functionality.

However implementing those hooks does not make any errors, because the original hook did not change, so implementing those hooks and using the provider with 1.3 will work fine, except of course the step wont be skipped, but thats not an issue.

As I see, as we keep the old hook as-is, this is the maximum what we can do in order to provide backwards compatibility. It's good to see that you made a great progress in this and came up with a simple and small patch to achieve this goal.

On the compatibility front, it's worth noting, for users of drupal_alters, that there are now two separate providers arrays.
Structure and label alterations made in hook_scald_atom_providers_alter() are propagated in the array returned by scald_atom_providers_opt(), but structure and label alterations made in hook_scald_atom_providers_opt_alter() are NOT propagated in the array returned by scald_atom_providers() (still used by scald_atom_add_form_source() and scald_views_handler_filter_atom_provider()).
Again, this is just a note...

Yes, the label should be overriden by hook_scald_atom_providers_alter()

and all the rest of the data should be overriden by hook_scald_atom_providers_opt_alter()

I guess that could be added to the comments in the functions and in the api documentation. Will add that to the patch.

That was a necessary compromise for simplicity but likely wont cause a problem if its well documented.

all the rest of ...

Not exactly : if a provider wishes to disable another provider (in the current context), he has to do it in hook_scald_atom_providers_alter() (what I called "Structure alterations")
For instance, this could be used to implement permissions at the provider's level...

Here is the new patch with doc updates.

Thanks Committed.