AdvancedCustomFields / acf

Advanced Custom Fields
http://advancedcustomfields.com/
871 stars 182 forks source link

Feature Request: acf/after_format_value filter #450

Open jan-herman opened 3 years ago

jan-herman commented 3 years ago

Hey @elliotcondon,

recently I found myself in need of formatting ACF values based on a condition which could change during the runtime and found out that this can't be achieved by using the standard acf/format_value filter due to the internal caching mechanism. The formatted value is cached the first time the function is called and if the conditon changes later on it has no effect.

I made it work by using the acf/pre_format_value filter to bypass the default logic inside the acf_format_value() function but this introduces potential problems down the line when you update the plugin.

That's why I'm proposing a new acf/after_format_value filter. (Or maybe acf/post_format_value but that might be confusing.)

It would look something like this:

function acf_format_value( $value, $post_id, $field ) {

    // Allow filter to short-circuit load_value logic.
    $check = apply_filters( "acf/pre_format_value", null, $value, $post_id, $field );
    if( $check !== null ) {
        return $check;
    }

    // Get field name.
    $field_name = $field['name'];

    // Check store.
    $store = acf_get_store( 'values' );
    if( $store->has( "$post_id:$field_name:formatted" ) ) {
        $formatted_value = $store->get( "$post_id:$field_name:formatted" );
        $formatted_value = apply_filters( "acf/after_format_value", $formatted_value, $post_id, $field );
        return $formatted_value;
    }

    /**
    * Filters the $value for use in a template function.
    *
    * @date 28/09/13
    * @since    5.0.0
    *
    * @param    mixed $value The value to preview.
    * @param    string $post_id The post ID for this value.
    * @param    array $field The field array.
    */
    $value = apply_filters( "acf/format_value", $value, $post_id, $field );

    // Update store.
    $store->set( "$post_id:$field_name:formatted", $value );

    // New filter
    $value = apply_filters( "acf/after_format_value", $value, $post_id, $field );

    // Return value.
    return $value;
}

Just for context this is my use case: I'm using the Latte templating engine and I need to format the values differently if the get_field function is called from the template .latte file.

I'm aware this is an edge case and there might be more elegant solution to my problem so feel free to ignore this if you think it's a stupid idea;)

elliotcondon commented 3 years ago

Hi @jan-herman

Thanks for the issue report and proposed solution. I'll be honest here and say that adding an "after" filter is not something that will likely make it into the plugin.

Instead, I would like to investigate different solutions to help. Can you explain a little more about the issue so I can understand why this "after" filter would solve it?

jan-herman commented 3 years ago

I understand. The code is not pretty and realistically I might be the only who needs this:)

But I would love to hear if you have any ideas how to solve my problem...

I'm using kinda unusual theme structure - it's like a hybrid between Sage theme with controllers (https://github.com/soberwp/controller) and Timber but using Latte (https://latte.nette.org/en/) as a templating engine instead of Twig.

Where Latte realy shines (among other things) is its context-aware escaping. But at the same time it introduces some problems. Like when you're trying to render some HTML blocks stored in the database. I.e. contents of WYSIWYG editor or any field that is formated using the wpautop() function for that matter.

The easy solution would to use the |noescape filter every time I'm rendering a field that might potentially contain any html. But that's neither pretty nor smart.

The other way is to turn this html into a special object which is than rendered as a regular html. And that is where the proposed filter comes to play.

In an ideal world I would do this:

function barista_acf_format_value($value, $post_id, $field)
{
    if (Barista::isRendering() && $value && is_string($value) && $value != strip_tags($value)) {
        return new SafeHtmlString($value);
    }

    return $value;
}
add_filter('acf/format_value', __NAMESPACE__ . '\\barista_acf_format_value', 10, 3);

Just check if the function was called from within the termplate (Barista is the Timber alternative / layer between WP and the templating engine) and if it contains any html. If it does I turn it into the aforementioned "magic" object.

But now imagine I use get_field('title') in my controller. The value is formatted the standard way (Barista::isRendering() returns false) and it's cached. Then I call it again from within the template (Barista::isRendering() returns true) and I get the same result because acf/format_value filter was ran just the first time.

The proposed filter would solve this problem.

Sorry for the lenghty explanation, but it's a very specific use case:D

elliotcondon commented 3 years ago

Hi @jan-herman

Thanks for the detail. This makes the issue much easier to understand.

If the solution here is to avoid ACF's cached $value interfering with your Barista::isRendering() variable, I believe the "acf/pre_format_value" filter will be a possible solution. This filter is run early in the acf_format_value() function and will allow you to either run your own logic for formatting values, or could act as an event where you clear the cached value.

I suggest spending some time reviewing the acf_format_value() function found within the includes/acf-value-functions.php file on line 112. With a little trial and error, you should be able to find a neat solution!

jan-herman commented 3 years ago

That is exactly what I did and it works just fine. Basically I copied the original code and added an extra bit at the places where the proposed filter would be.

I'm just concerned that in the future you might do some changes in the caching mechanism and when the client updates the plugin it might break the site.

But I guess I could add some safety checks. Like make sure that the acf_get_store() function exists etc. Then I could just throw an exception instead of getting an error.

Anyway thanks for your input and keep up the good work;)

elliotcondon commented 3 years ago

@jan-herman Perfect! Yes, it's always a good idea to check that undocumented functions exist. I'll keep thinking about this issue in the background. Perhaps there is a way we can allow the "cache name" to be customized with additional variables? This way, your Barista::isRendering() flag could simply integrate at that stage and allow all the other core code to continue working as normal.

jan-herman commented 3 years ago

I tried leveraging the caching mechanism and for now it looks like this:

function barista_acf_format_value($null, $value, $post_id, $field)
{
    $field_name = $field['name'];
    $barista_is_rendering = Barista::isRendering();

    $store = acf_get_store('values');
    $formatting = $barista_is_rendering ? 'barista' : 'formatted';

    if ($store->has("$post_id:$field_name:$formatting")) {
        return $store->get("$post_id:$field_name:$formatting");
    }

    $value = apply_filters('acf/format_value', $value, $post_id, $field);

    $store->set("$post_id:$field_name:formatted", $value);

    if ($barista_is_rendering) {
        if ($value && is_string($value) && $value != strip_tags($value)) {
            $value = new SafeHtmlString($value);
        }

        $store->set("$post_id:$field_name:barista", $value);

        return $value;
    }

    return $value;
}
add_filter('acf/pre_format_value', __NAMESPACE__ . '\\barista_acf_format_value', 10, 4);

I'll definitely try to future proof it little bit but in theory it should work as long as the acf_get_store() function exists and the returned ACF_Data object has the the get(), set() and has() methods which are pretty standard and I don't think they are likely to change.

jan-herman commented 3 years ago

I thought about what you suggested (using additional variables to change the "cache name") and I came up with this:

function acf_format_value( $value, $post_id, $field ) {

    // Allow filter to short-circuit load_value logic.
    $check = apply_filters( "acf/pre_format_value", null, $value, $post_id, $field );
    if( $check !== null ) {
        return $check;
    }

    // Get formatting.
    $formatting = 'formatted';
    $formatting = apply_filters( "acf/formatting", $formatting );

    // Get field name.
    $field_name = $field['name'];

    // Check store.
    $store = acf_get_store( 'values' );
    if( $store->has( "$post_id:$field_name:$formatting" ) ) {
        return $store->get( "$post_id:$field_name:$formatting" );
    }

    /**
    * Filters the $value for use in a template function.
    *
    * @date 28/09/13
    * @since    5.0.0
    *
    * @param    mixed $value The value to preview.
    * @param    string $post_id The post ID for this value.
    * @param    array $field The field array.
    * @param    string $formatting The formatting flag.
    */
    $value = apply_filters( "acf/format_value", $value, $post_id, $field, $formatting );

    // Update store.
    $store->set( "$post_id:$field_name:$formatting", $value );

    // Return value.
    return $value;
}

I added the acf/formatting filter which would be used to change the "flag" in the cache key name. (In my case changing it to "barista" when Barista::isRendering() is true.)

And I'm passing the $formatting variable to the current acf/format_value filter where I could then apply the custom logic.

It could use some polishing but I think is's pretty elegant. But I understand if it's not something you want to implement. Probably only a handful of people will find the need for this.

Zealth57 commented 3 years ago

Just want to throw in that I struggled with this exact same problem and would fine it very useful to find a more elegant and easy way to bypass the cache store. I have options set in a site options page, and then I also have those same fields copied and used in pages so users can override settings on a per page basis. For example you upload a logo image into an ACF options page, and then you create a new page called "Hello World" and in the sidebar there are page level ACF fields that you can upload a logo to. I wanted a way to dynamically replace the values on the fly using the format_value so I could create new options / page level fields and it would work automatically. I ended up having to use the format_value myself on the value and then hook into pre_format_value to do the actual replacement which feels really jenky.