Convert entity_field to use #ajax for formatter options

Subscribe

I fear the panels ui is not compatible with the default ajax callback.

dereine is correct. #ajax makes too many assumptions about the form to be used in Panels forms at this time.

For example, that the form callback will always be available. :(

Move to ctools as the fix is there.

Sadly the configuration is not stored at the moment. Any kind of idea why would be actually really great!

To easy reproduce the issue just add a body field and choose trimmed as formatter and use another formatter.
Not even the formatter is stored so i'm a bit confused.

No idea whether this is elegant or not but this two patches works for me.

/me bets there are millions of dsm's left.

Some less dd() calls maybe some rewrite of the fields integration could be removed again.

Here is the newest version.

Manuall testing of the field-field worked fine.

PS: We need a easy way to remove many fields, like on views :)

Here is a new version for ctools.

Hello,

Got the same issue with Form API Ajax & Ctools (using a custom form in a custom ctools content type. The custom form isn't foundable as it's defined in content type file which is not included). Is there any news on this task since September ? May I assume applying these patches is enough to fix it ?

I have patched against panels-3.0-alpha3 (need it for a site currently on developing stage), because the latest dev version of panels did not include the changes described there.

So only #10 was required but it's part of panels now.

Here is a patch

[merlin@furor ctools (7.x-1.x)]$ git apply 1259430-entity_field-formatter-ajax.patch
1259430-entity_field-formatter-ajax.patch:31: new blank line at EOF.
+
error: patch failed: includes/fields.inc:80
error: includes/fields.inc: patch does not apply

I rerolled the patch, it should apply against the actual git (7.x-1.x ) branch. Would be awesome to get this commited!

I should attach it as well :P

Everything seems to work functionality wise.
Tested with a vanilla installation as well as existing ones - some of them with quite complex formatters (media & slideshow stuff).
However, I've found some coding standard violations in the code touched by the patch.
I've adjusted that - please check interdiff to see the changes.

Looks like something went wrong. Let's try again.

I'm about to re-factor a bigger site (thus there's this issue too: #2011814: Add a JS quick filter to the add content modal dialog ;) ) - I'll apply the latest patch and will do extensive manual testing that way.

While working with the patch applied I came across some issues.
The values/fields defined by ctools_content_configure_form_defaults() weren't stored if the formatter was changed.
Reason was, that ctools_content_configure_form_defaults_submit() wasn't called by the ajax request.
The attached patch fixes that.
However, if someone knows of a more generic way to solve this hints would be very welcome :)

Seems reasonable to me. Thanks for working on this! For me this loos RTBC, would be very nice to see this committed :)

Up

Re-roll and fix for default formatter settings.

Coud the patch be commit or something is preventing it?

if (!empty($info['settings'])) {
foreach ($info['settings'] as $field_name => $value) {
- if (isset($form_state['values'][$field_name])) {
- $form_state['conf']['formatter_settings'][$field_name] = $form_state['values'][$field_name];
+ if (isset($form_state['values']['settings'][$field_name])) {
+ $form_state['conf']['formatter_settings'][$field_name] = $form_state['values']['settings'][$field_name];
}
}
}

Please, do use here "early return" pattern?

if (!isset($form_state['values'][$field_name])) {
  continue;
}
if (!isset($form_state['values']['settings'][$field_name])) {
  continue;
}
$form_state['conf']['formatter_settings'][$field_name] = $form_state['values']['settings'][$field_name];

@podarok What would be the gain of that, and should we really adjust the architecture here? That's somehow an unrelated change as far as I can see atm. And it's already taking long to get this in without "unrelated" changes ;)

You were asking for review. I did that.
If You do not want to get this commited, just ignore my review, who need that... (((
http://pear.php.net/manual/en/standards.bestpractices.php

@merlinofchaos, If You are good with this spaghetti code, please, do commit it.

@podarok Hey, easy there. What's wrong with asking what the benefit of something is? This wasn't any-kind of attack towards you. The issue queue is a place to discuss erm well issues and that's all I wanted to do. If my response looked like an attack to you, I sincerely apologize.
And of course reviews are highly appreciated, but that doesn't mean one isn't allowed to ask questions about a feedback.

Now back on the technical / administrative topic:

• Often we've to hold back nice to have or even required changes to get things done. I've heard to many times "unrelated change -> revert" to just make changes that could be declared in such a way.
• The piece of code we're talking about, the function ctools_entity_field_content_type_formatter_options_submit (as far as I understand) has 22 lines and 2 nested foreach loops (9 lines in total). For me this is far from unreadable and very well maintainable without adding further flow controls to enhance (if even) the readability.
• Regarding the best practise, I've heard equally often pros and cons for early returns / other early flow controls to categorize them as "use whenever it makes sense" and not as "requirement".
  In the current case I'm in doubt that it makes sense because it's such a small piece of code. However, I guess this is a topic we could discus days and may would have to agree that we disagree :)

So based on that I asked "What would be the gain of that..."

Finally I think all the information needed is available, still reviews needed & very welcome - but be warned there might be questions related to incoming feedback :) That said, back to "Needs review".

Re-rolled

rerolled against current dev

Have been using #31 for a while and does the job for us.

Hey ron, thanks for the info! It's really helpful to know when things work.

Just checked and the patch applies cleanly to current ctools 7.x-1.x-dev.

There's some good work in the function-docs and the code seems to be sane on a quick read-through, though I've done no detailed checks.

@das-peter, @ron_s: It would be really helpful if there were some additional 'simpletest' tests for this area... even something small to get started with.

Ugh. This -looks- ok to me, but it is a lot of code. Perhaps we can get a test to help confirm its working?

This didn't get into 1.13, maybe it will get into 1.14.

We need to try getting this to RTBC.

There was a change to the ctools_fields_get_field_formatter_settings_form function in 1.13 that causes this patch to break.

The difference between #31 and #39 is the function no longer has if conditionals around it (this has been removed: https://cgit.drupalcode.org/ctools/tree/includes/fields.inc?id=2047a9b91...), so the if (!empty($form_key)) { patch code isn't referenced correctly.

New version available for review, see attached.

Patch #39 still applies cleanly to the new CTools 7.x-1.14.

Anyone available to test so we can possibly get into the next release?

Still applies cleanly to 7.x-1.15...

I agree with @jperry, #35 needs tests, I don't feel confident committing this without them.

We've been using patch #39 for the past 2.5 years with no issues, but I understand if tests are preferred. Unfortunately I don't have availability at the moment to do so.