vinkla / extended-acf

Register advanced custom fields with object-oriented PHP
MIT License
455 stars 61 forks source link

Add customFormat method to Field class #152

Closed kaelansmith closed 3 weeks ago

kaelansmith commented 3 months ago

This is a feature idea/request that I figured would be easier to share via a PR. Here's the gist: I would like to make it easy to customize the response format of any given field via a customFormat field method (open to better name suggestions); this method would be available to ALL fields, and is therefore defined in the root Field class. It simply leverages ACF's acf/format_value/key={field_key} filter hook under-the-hood -- so you pass a callback function into customFormat in the same way you would pass it to that filter hook. When the get method runs for a field, it checks if a customFormat callback was provided, and sets up the filter for you using the auto-generated (or user-provided) field key.

Example:

Relationship::make('Related Project')
     ->postTypes(['project'])
     ->customFormat(function ($value, $post_id, $field) {
            if (!is_array($value))
              return $value;

            return array_map(function ($project) {
              return [
                'id' => $project->ID,
                'post_title' => $project->post_title,
                'custom_field' => 'whatever'
              ];
            }, $value);
      })

This is particularly valuable for the types of projects I build, which are headless/decoupled sites where WP is essentially just a REST API that my JS frontends consume; I often need to tweak ACF field formats to expose stuff to the REST API that isn't included by default, or to remove unnecessary nested properties that bloat my API request payload sizes etc. (like my example above which trims out a bunch of bloat that Relationship fields often include). I have so far been using the acf/format_value filter hook to adjust field types globally (i.e. add_filter('acf/format_value/type=image' ...);), but I have recently come across the need to adjust formatting for specific/one-off fields; I can manually set up the filter hook for a given field if I manually provide a key to the field via the key method, but I would prefer to continue leveraging the auto-generated field keys and I find the customFormat method much cleaner in general.

Thoughts?

vinkla commented 3 months ago

This is an interesting idea, Kaelan. It's worth exploring further. The naming convention could be improved. The name customFormat may not be the best choice, and the format method name is already used in several fields, such as the date picker. Perhaps we could take inspiration from Filament or Laravel Nova for a more suitable name.

vinkla commented 3 months ago

The format method name can still be explored. To support callable functions, the DateTimeFormat trait will need to be updated, but this should not pose a significant problem.

The format method should not be available on all fields. It doesn't make sense on fields like message or tab. The question is whether it makes sense on fields such as repeater and flexible content.

kaelansmith commented 3 months ago

Yes I actually took inspiration from the existing format method, because that method also modifies the response format of a field using one of a few "presets" -- so customFormat made sense in my brain because it's doing the same thing, just using your own "custom" formatting logic.

I originally was thinking the callback functionality could be merged into the existing format method, but I noticed that format is manually defined on 14 fields (not just via the DateTimeFormat trait), so I decided it would be simpler to include it in one global place (Field) -- but you're right that it shouldn't be available to Tab or Message fields, I didn't think about that. As for what fields it should be included on, perhaps any field that holds a value AND that doesn't have sub_fields (i.e. exclude tabs, accordion, message, repeater, flexible_content, and group); I don't see why I would ever adjust the data structures of fields with sub_fields -- if I wanted to adjust one of their sub_fields, I would just use customFormat on the sub_field.

In case you do want to separate customFormat from format, I asked ChatGPT for naming suggestions based on Filament/Laravel Nova:

  • formatUsing: This name aligns well with the convention of method names in Laravel and is straightforward about its purpose.
  • transformValue: This name implies that the value is being altered or transformed by the callback.
  • formatWith: This is similar to formatUsing but emphasizes the use of an external function or callback.
  • applyFormatting: This name makes it clear that formatting is being applied to the field value.
  • valueFormatter: This clearly indicates that the method involves formatting the value.
nikrowell commented 3 months ago

Hi Guys -

What if this was approached form a more generic angle? Rather than having a method specific to format_value, there could be 2 new methods for action($hook, $callable) and filter($hook, $callable) that accept any of ACF's field-specific actions or filters.

I'll sometimes use load_value to return default values:

add_filter('acf/load_value/name=page_headline', function($value, $post_id) {
    return (is_admin() || $value) ? $value : get_the_title($post_id);
}, 10, 2);

... or prepare_field to modify settings or even hide a nested subfield completely on edge cases.

This could be a Extended\ACF\Fields\Settings\Hooks trait, or kept simple as a Field method knowing that (1) the majority of fields would use this and (2) fields like message or tabs simply won't do anything (just as an invalid action or filter name would do nothing).

kaelansmith commented 3 months ago

@nikrowell I could see that being nice for filters. But for actions, it looks like the only field-specific action is acf/render_field -- it wouldn't really make sense to define any other ACF action on a specific field definition since they're global in nature. So it almost makes sense for that single action to have a standalone method, maybe called onRender -- or even two methods renderBefore and renderAfter.

The filter($hook, $callable) method could be a nice way to initially roll-out support for this, and maybe in the future it could make sense to have unique methods for certain filters where extra abstraction is helpful. For example, at first glance it might be nice to add a layer of abstraction around the user-provided callbacks for load_field and prepare_field, where we pass in the field class instance so the user can modify field settings via Extended ACF methods rather than directly editing the raw settings array:

Text::make('Heading')
  ->filterSettingsOnLoad(function ($field, $fieldObj) {
    if ($field['some_setting'] == 'some_value') {
      return $fieldObj->required()->helperText('Special instructions...');
    }
  });

// in filterSettingsOnLoad:
filterSettingsOnLoad($callable): static {
  // the filter below would actually get set in `get`, where the final `key` is known
  add_filter("acf/load_field/key={key}", function($field) {
    $userResult = $callable($field, $this);
    if ( ~ is $userResult an Extended/ACF class instance? ~ ) {
      return $userResult->get();
    }
    return $userResult;
  });

  return $this;
}

^ That might not be a great example, but the point is that some filters may warrant some unique abstractions, in which case having a standalone method is desirable.

vinkla commented 3 months ago

@kaelansmith, you're right, it would require some work adding support on the 14 fields. We could also explore names such as render or value. Let's think about the naming a bit more. Perhaps there is another name we could use.

@nikrowell, I agree with this idea as it would make registering filters on fields much cleaner. Keeping the filter close to the field avoids the need to switch contexts.

Text::make('Heading')
    ->maxLength(100)
    ->filter('acf/load_value/key', function (mixed $value, int|string $id): string {
        return (is_admin() || $value) ? $value : get_the_title($id));
    })
    ->required();

The syntax for these methods should be carefully defined. The filter method would serve as a wrapper, supporting both simple filters, such as formatting the value, and more complex ones. Additionally, users should have the option to omit the /key=field_123 part of filters, allowing them to add it directly through the method. Alternatively, as @kaelansmith suggested, adding some kind of templating for the filters, such as /key={key}, could be a viable solution.

I also wonder which filters we should support or simply let be. Anything unrelated to the current field would not make sense to support, in my opinion.

kaelansmith commented 3 months ago

@vinkla I like the simplicity of render (I assume you're referring to the proposed wrapper method for the acf/render_field action hook here?) and value -- but they do seem a bit vague. I'm personally leaning towards renderBefore and renderAfter the more I think about that one, where renderBefore sets a hook priority less than 10, and renderAfter does the opposite -- I think it would be nice to abstract that difficult-to-remember aspect. I guess it could also just be render(function($field) { echo '<p>Some extra HTML.</p>' }, 'before') (i.e. 2nd argument). renderWith also makes sense in my brain, because you're adding to the default field rendering instead of completely overriding it -- whereas value completely overrides the default value.. I have a feeling I might come around to value actually.

Additionally, users should have the option to omit the /key=field_123 part of filters

I'm not sure why/when a user would ever want to exclude that part? I would think that defining a hook through one of these proposed field methods should always scope the hook to that field -- if I wanted the filter to apply more globally, I would define the filter as usual, separate from the field.

I also wonder which filters we should support or simply let be.

Besides the ones already mentioned (acf/format_value, acf/load_field, acf/load_value, acf/prepare_field) I would think any of the acf/fields/* filters should be supported (where acf/fields/relationship/result would only be available to Relationship fields, etc.); it would make sense to also support acf/update_field, acf/update_value, acf/upload_prefilter, acf/validate_attachment, and acf/validate_value -- basically any filter that supports the /key={field_key} modifier.

Sorry this idea has ballooned into a big endeavour, lol.

vinkla commented 3 months ago

I'm not sure why/when a user would ever want to exclude that part?

Since the package generates the keys automatically, we should provide an implementation that adds the field key from the method rather than requiring the developer to add it themselves.

Besides the ones already mentioned (acf/format_value, acf/load_field, acf/load_value, acf/prepare_field) I would think any of the acf/fields/* filters should be supported (where acf/fields/relationship/result would only be available to Relationship fields, etc.); it would make sense to also support acf/update_field, acf/update_value, acf/upload_prefilter, acf/validate_attachment, and acf/validate_value -- basically any filter that supports the /key={field_key} modifier.

Let's support everything in the first version. Would you like to create a pull request, @kaelansmith?

Sorry this idea has ballooned into a big endeavour, lol.

I don't think this will be a big endeavor. It should be straightforward adding a filter method and applying the field key automatically.

kaelansmith commented 3 months ago

@vinkla ah, we're on the same page then -- I misunderstood what you meant.

Sure, I can likely find time for a PR soon. To clarify, you'd like a single filter method, but also to enforce limitations on a per-field basis so that only the relevant filters can be used, right?

vinkla commented 3 months ago

Yes, that would be great. But I wonder if it's really necessary to limit it to the current field what do you think? Should we allow all filters from the start?

vinkla commented 3 weeks ago

Closing this in favour of creating a filter method instead.