search

Islandora-Collaboration-Group / islandora_webform

islandora_webform
GNU General Public License v3.0
6 stars 8 forks source link

Proposed solution for issue-14 #18

Closed DiegoPino closed 8 years ago

DiegoPino commented 8 years ago

Refactored iwi_set_form_element_byhash, now accepts a numeric offset for appends. Remove $cloned_elements for function definition and call, because that variable makes only sense in a local call scope. If using an "alter" it will always be empty.

If mode == "append" and $offset == 0 then it will replace If mode == "append" and $offset == 5 then it will create new child elements to fulfill the request and will put the default value in the last one. Subsequent calls will fill already created elements if $offset <=5

$offsets worka as position inside a form child element array starting from 0. So offset 5 means 6 elements.

Note: only code changes passed drupal coding standards.

patdunlavey commented 8 years ago

Hi @DiegoPino Just began testing today. I'm getting white screen when I use the "append" option. Acting like out of memory, but I haven't had a chance to drill down into it. Curious if you have seen anything like that. Also added that drupal_alter call and am getting an error from Tab:FilterChildren() that '#title' is not defined.

I'll do more testing/digging!

DiegoPino commented 8 years ago

No, i did not get that. But i had to clear caches, Can you post me an example form? for new objects or existing datastream?

On Oct 7, 2015, at 3:17 PM, Pat Dunlavey notifications@github.com wrote:

Hi @DiegoPino Just began testing today. I'm getting white screen when I use the "append" option. Acting like out of memory, but I haven't had a chance to drill down into it. Curious if you have seen anything like that. Also added that drupal_alter call and am getting an error from Tab:FilterChildren() that '#title' is not defined.

I'll do more testing/digging!

— Reply to this email directly or view it on GitHub.

DiegoPino commented 8 years ago

@patdunlavey, also can you show me your call? (is the path complete with the [0]?). I have some doubts, but i think you can't do normal orm alter, you need to alter your form builder function or at least have the objective form storage object present and alterable, because once the objective form is initialised things are way more complicated.

patdunlavey commented 8 years ago

I'm thinking about a possible change. Instead of thinking of $offset as a counter, just think of it as an ID. If $offset exists, then write to that element. If it doesn't exist, clone a new element and write to it. In this scenario, 'replace' simply means write to the element at offset=first kid; 'append' means create a new element after last kid; offset means write to the element with that ID.

I'll write more later when I get time to pull more thoughts together.

DiegoPino commented 8 years ago

Sadly objective forms don't work that way. The whole clone stuff depends on consecutive indexes.

patdunlavey commented 8 years ago

This is very confusing - there is a lot of code in this function that I cannot understand how it works! Nothing ever gets written back to $form or $form_state that I can see (I don't think I see any assignment by reference, and there is no explicit assignment). The function calls $element->adopt(), in which, incidentally you could add an index (e.g. $element->adopt($new_tab, $offset)). It also does some array assignment logic to add the form elements to $special_element_ref. In neither case can I see how this is getting assigned back into the $form or $form_state, but in both cases, I do think it is possible to set the indexes explicitly. Obviously I am missing something fundamental about how this all works! - The consecutive indexes thing is just part of my confusion!

DiegoPino commented 8 years ago

Patrick, yes it's a not easy to follow function, and yes, ->adopt allows for a key. The problem i'm trying to resolve is not tied to that condition, but the way the adding and removing of a new tab happens via ajax during a form view. If that "add button" where not there then you could basically add an arbitrary 'key' for that specific form element because the paths where fixed. But what happens is that if you add and remove during the editing the child tabs get reordered and you loose your original keys (and then also your values). Citing in the original function, because my pull is not merged in issue-14.

You are looking where the values are appended to the form:

https://github.com/commonmedia/islandora_webform/blob/issue-14/submodules/islandora_webform_ingest/includes/utilities.inc#L444 $ref is a reference to $form https://github.com/commonmedia/islandora_webform/blob/issue-14/submodules/islandora_webform_ingest/includes/utilities.inc#L475 $special_element_ref is a reference to the original form element we are cloning So there it is, since we have 2 references at the end assigning to those passed to $form. $form_state is indirectly modified because the objective form storage is being read and updated from there.

But as i said, i already removed the the "for as it exists currently", i do now a array_keys to values and then iterate and will add this to the current pull during the weekend. And i also iterate because you could be targeting a form that has a "template", already full of data or a form filled with data from a datastream. If you get a template full of data (lets say 3 tabs) how should the xml form khow to give those 2 extra tabs a key? well it does not, so it just appends at the end, which means consecutive numeric keys.

If you want we can walk through the function together so it get easier to understand

patdunlavey commented 8 years ago

Ah, I see how $special_element_ref connects back to $form. Sorry for being dense, and thank you for your patience!

The add button only affects the element numbering after the form has been displayed. Aren't we only concerned about our indexes while we are building/altering the form?

In any case, I don't think there is a use-case for filling in empty place-holder form elements when the offset is 2 or more greater than last-kid. I can't think of any reason why would we ever actually want to do that.

DiegoPino commented 8 years ago

@patdunlavey , pulled, look at the https://github.com/DiegoPino/islandora_webform/commit/5f23c21e813560f4a8bb5df597d6bca797708285 description. Makes what you want, puts a clone wherever you need, some little things changed. Tested, not strange loops.

Side note: since you are now targeting 'any' offset, just remember: Arrays with mixed keys can be accessed using numeric keys and/or string, to it's up to you to keep the order when altering and not ending overwriting /replacing instead of appending.

patdunlavey commented 8 years ago

Hi @DiegoPino. Replace seems to work good, and append works in the most basic use-case: fill in one field in a new tab. But things get a little wacky when I try other things.

In standard field mapping, "append" always adds a tab, whether or not there is anything in the first tab, and whether or not append was previously run in another field mapping. Each time you map a field in append mode, you create a new tab - you can't map values to multiple fields in the same tab (e.g. name and name type).

I'm having trouble getting my custom alter hook to do what it's supposed to (see where I implemented it here: https://github.com/patdunlavey/islandora_webform/blob/feature/ingest_form_alter_hook/submodules/islandora_webform_ingest/includes/ingest_page.inc#L195). There needs to be a way to unset/remove or overwrite the results of built-in mapped fields in our alter hook.

For this testing, I have four fields: corporate name, corporate type, personal name, personal type. Corporate name and type are mapped to two fields in the name tab in "replace" mode. The personal name and type fields are mapped to the same destinations in "append" mode. (This configuration was the closest I could do to get it to work sort of as expected. Setting everything as "append" just creates a new tab for each field.) Without enabling my alter hook, I get the first tab populated as before (since it's in replace mode, both fields are correctly populated with the form input). Two more tabs are created: one for each of the two append mappings (the problem described above but which we'll ignore for the moment).

I'll be digging into this further in the morning, but if you have any thoughts, please let me know!

DiegoPino commented 8 years ago

@patdunlavey, i think you have an implementation problem related to the fact that you are handling now mixed keys which is a problem by itself.

First problem i see: your alter https://github.com/patdunlavey/islandora_webform/blob/feature/ingest_form_alter_hook/submodules/islandora_webform_ingest/includes/ingest_page.inc#L195 is at the wrong place or at least in a very dangerous place, because on every ajax call it will modify the $form and you want that alter to run only on $form2 when the forms are being build in the first place, so it should be between lines 178 and 177 …also your "paths" are not correct anymore in the whole $form context, am i right? They are specific to the $form2 context. This is very important.

Second problem: you test form, the reason i re-wrote that function is because mixed keys are a problem so i had to find a solution for that specific case. The current default for $offset is NULL, means just append and keep appending. If you give your $offset an initial number (not 0…maybe something bigger like 100?) or "string" in standard field mapping, "using append" then you get your functionality back. Since we don't know for sure what name did you give to a special element container in your original form without running the function, we a few choices there:

Since every call will be made with that offset, calling two appends to the different childs of the same special container ,with the same offset, will just add the value the second time to the existing one.So you get no new tabs added. Only one for the standard implementation. You have your previous functionality.

If I start my alter hook using an offset of 0, and increment from there, I'm only getting three tabs created. The first is the one created by the unaltered form (whose index, I think, was 'namePart' ). The second and third tabs (which are at indexes 0 and 1) contain what the desired results for what should be the first and second tabs.

True: [0] index is the first child always if you don't have a explicit [0] there (and your first one is named "nameTab", that can be accessed in php also by the 0 key). The problem with mixed keys! (I explained that in the pull) So if you append with an existing $offset (0 is there) you just replace the values for the main form because this will match to the else : https://github.com/DiegoPino/islandora_webform/commit/5f23c21e813560f4a8bb5df597d6bca797708285#diff-0e084d5456af8c4d39ea0024b72e3cafL507

So…you are stuck to string literal indexes or you assume a big number hoping there are no other previous filled tabs and tags there.

But if I start with an offset of 3, then I get eight tabs, the first three which are the originals, and the last five which are the correctly altered ones. It appears that the inserts work properly when the starting offset is greater than any existing one. But that leaves the old tabs, which we don't want. I think getting the five added tabs to work (in this test case), regardless of whether we are starting at an existing offset, has to be part of the solution. The second part is to figure out a way to remove the results of the original field mappings - possibly by writing a new function, or by implementing additional modes (e.g. "remove", "nullify", etc.) in this function.

Here again we have a problem. Is $offset the number of tabs of tags you want? Or the position inside an existing child elements array(that sadly is mixed in this case?). You need to clarify that aspect. Because assuming $offset is a quantity makes "targeting" a blind procedure.

Anyway, we still could have choices, but i'm not sure now what the expected functionality is, ideally would to keep track in the alter implementation of what you have and what you want to add.Also, Using the default $offset = NULL is always a risk when dealing with mixed keys, because even when you already have like 5 child elements (lets say they where named by you nameTab1---nameTab5), the next one on a simple, plain append without offset will be [0], which moves your whole referencing and makes stuff a lot more complicated….but if your form starts with [0] then the next one will be [1].

patdunlavey commented 8 years ago

First problem i see: your alter https://github.com/patdunlavey/islandora_webform/blob/feature/ingest_form_alter_hook/submodules/islandora_webform_ingest/includes/ingest_page.inc#L195 is at the wrong place or at least in a very dangerous place, because on every ajax call it will modify the $form and you want that alter to run only on $form2 when the forms are being build in the first place, so it should be between lines 178 and 177 …also your "paths" are not correct anymore in the whole $form context, am i right? They are specific to the $form2 context. This is very important.

I moved the drupal_alter call to line 178 - is this right?: `

  // Only do this once. If ajax is called, then the form is rebuilt and
  // we end re-adding new elements. So check if form was rebuild. If yes,
  // don't do anything.
  if (!isset($form_state['rebuild']) || $form_state['rebuild'] == FALSE) {
    // Loop through the submitted values, setting them into $form2.
    foreach ($dsid_values['data_values'] as $input) {
      iwi_set_form_element_byhash($form2, $form_state_new, $input['path'], $input['input_value'], $input['mode']);
    }

    // Permit other modules to modify the form.
    drupal_alter('iwi_set_form', $form2, $form_state_new, $submission);
  }

`

I'm not sure what is the correct way to set up the $elementspath variable for this context. I assumed the form path would not change. I tested putting actual offsets into this variable, e.g.

iwi_set_form_element_byhash($form, $form_state, 'nameTabs:' . $offset . ':nameSet:nameType', $input_value, 'append', $offset); but that threw a bunch of errors for offsets >= 2 (i.e. unmatched by previously created offsets). I'm not sure what else you might mean.

If you give your $offset an initial number (not 0…maybe something bigger like 100?) or "string" in standard field mapping, "using append" then you get your functionality back. Since we don't know for sure what name did you give to a special element container in your original form without running the function, we a few choices there:

Unfortunately, this doesn't work 100%: When I set the offset to start at 4 or more, or if I use string offsets (e.g. 'namePanel0', 'namePanel1', etc), only the last field written at each offset comes through. I.e. if I write to both nameType and namePart at an offset, only the second (namePart) actually appears in the form. So the five new tabs are only created correctly if I set the initial offset as a number that is exactly the number of previously created tabs.

Here again we have a problem. Is $offset the number of tabs of tags you want? Or the position inside an existing child elements array(that sadly is mixed in this case?). You need to clarify that aspect. Because assuming $offset is a quantity makes "targeting" a blind procedure.

$offset is always intended to be a position. Easy to provide that as either string or number, but no, it's never a quantity. So targeting, at least for tabs we are writing to in our alter hook, is not blind. However, figuring out how to remove or alter previously added tabs at unknown positions is a problem. Continuing...

Anyway, we still could have choices, but i'm not sure now what the expected functionality is, ideally would to keep track in the alter implementation of what you have and what you want to add.Also, Using the default $offset = NULL is always a risk when dealing with mixed keys, because even when you already have like 5 child elements (lets say they where named by you nameTab1---nameTab5), the next one on a simple, plain append without offset will be [0], which moves your whole referencing and makes stuff a lot more complicated….but if your form starts with [0] then the next one will be [1].

As it said, it's easy to keep track of the offsets we're writing to (overwriting or creating) in our implementation of the alter hook. I think it's reasonable to ask the implementer to be responsible for that. Not easy to know what is already there before the alter hook is called, however.

I added islandora_webform_ingest.api.php with an implementation of the alter hook so that you can see how I'm using it: https://github.com/patdunlavey/islandora_webform/blob/feature/ingest_form_alter_hook/submodules/islandora_webform_ingest/islandora_webform_ingest.api.php

patdunlavey commented 8 years ago

Hi @DiegoPino - I have a working alter hook!

See the last few commits: https://github.com/patdunlavey/islandora_webform/commits/feature/ingest_form_alter_hook.

The main one is https://github.com/patdunlavey/islandora_webform/commit/36d907950c4beb41b74d9de9debe9655caa2d70b in which I figured out how to remove the original nameTabs.

I don't think there is any need for putting the $cloned_elements into $form_state - unless you can think of another reason!

DiegoPino commented 8 years ago

Looking at this you don't need then my new code!! You got it. Yeah, the unique id string idea is the key (literally) and the way to remove the existing tab is perfect. The only thing i think we could improve is to make removing an existing element by path a direct function…should i go for that? If not, it's OK! Congrats

DiegoPino commented 8 years ago

Superseded by #26