Adding removeField and modifyField to GroupBuilder + Tests

[Androlax2]

Hi,

It should close this PR #91

Added unit tests for the GroupBuilder modifyField and removeField

[marcelo2605]

Hey @Androlax2, thanks for your collaboration. I'm trying to use the PR #91 to modify a repeater subfield without success. Could you take a look on the issue #141?

[Androlax2]

Hey @marcelo2605, could you send me all the code please.

$accordionModule = new FieldsBuilder('accordion-module');

$accordionModule->addFields(get_field_partial('partials.partial-load-content'));

$accordionModule->modifyField('title', function($fieldsBuilder) {
    $fieldsBuilder->setConfig('wrapper', ['width' => '77%']);
    return $fieldsBuilder;
});

Give me this part :

get_field_partial('partials.partial-load-content')

I will make some tests to see where is the problem.

[marcelo2605]

Sure @Androlax2. This is the content of partial-load-content file.

<?php

namespace App;

use StoutLogic\AcfBuilder\FieldsBuilder;

$loadContentPartial = new FieldsBuilder('load-content');

$loadContentPartial
    ->addTrueFalse('load_content', [
        'label' => 'Load content?',
        'wrapper' => [
            'width' => '33%',
        ],
        'ui' => 1,
    ])
    ->addSelect('select_post_type', [
        'required' => 1,
        'wrapper' => [
            'width' => '67%',
        ],
        'allow_null' => 1,
    ])
        ->conditional('load_content', '==', '1')
            ->and('select_posts', '!=', '1')
    ->addTrueFalse('filter_taxonomy', [
        'label' => 'Filter by taxonomy?',
        'wrapper' => [
            'width' => '33%',
        ],
        'ui' => 1,
    ])
        ->conditional('load_content', '==', '1')
            ->and('select_posts', '!=', '1')
    ->addSelect('select_taxonomy', [
        'required' => 1,
        'wrapper' => [
            'width' => '33%',
        ],
        'allow_null' => 1,
    ])
        ->conditional('load_content', '==', '1')
            ->and('filter_taxonomy', '==', '1')
    ->addSelect('select_term', [
        'required' => 1,
        'wrapper' => [
            'width' => '33%',
        ],
        'allow_null' => 1,
    ])
        ->conditional('load_content', '==', '1')
            ->and('filter_taxonomy', '==', '1')
    ->addTrueFalse('select_posts', [
        'label' => 'Select posts?',
        'ui' => 1,
    ])
        ->conditional('load_content', '==', '1')
    ->addPostObject('selected_posts', [
        'required' => 1,
        'post_type' => [],
        'taxonomy' => [],
        'multiple' => 1,
        'return_format' => 'id',
        'ui' => 1,
    ])
        ->conditional('load_content', '==', '1')
            ->and('select_posts', '==', '1')
    ->addRepeater('items', [
        'required' => 1,
        'min' => 1,
        'layout' => 'block',
        'button_label' => 'Add item',
    ])
        ->conditional('load_content', '!=', '1')
        ->addText('title', [
            'required'     => 1,
        ])
        ->addWysiwyg('description', [
            'required' => 1,
            'toolbar' => 'basic',
            'media_upload' => 0,
        ])
    ->endRepeater();

return $loadContentPartial;

[Androlax2]

@marcelo2605 Could you try with this fix please ?

I added the possibility to use a dot notation to go through the items.

You would do something like this now

$subject->modifyField('items.title', [
            'wrapper' => [
                'width' => '77%'
            ]
]);

I don't know if the mainteners will accept this.

If yes, we will need to refactor this, I did it just to make the test pass. Only one level deep is tested for now.

[marcelo2605]

@Androlax2 its works in the way you mentioned. But when I try to use a function, it return a FieldNotFoundException: Field items.title not found.

[Androlax2]

@marcelo2605 Yeah it's harder to do it with the closure. Did not find an easy way to do it for now.

[Androlax2]

@stevep If you are ok, I can deep the code and do something with this dot notation with tests etc... It may be nice to have this deep level modify/remove method for this package.

[capellopablo]

Hey guys... any update for this? I have a launch in some days and it would be great to have this PR in place to do not harcoding the package 😄 .

I think this will be really helpful.

[stevep]

@Androlax2 thanks for doing this, I think this is great and will help people.

The code and tests looked good to me, I merged it.

I'm not sure what you meant by your last message though, the dot notation was present in the 2nd commit

If you are ok, I can deep the code and do something with this dot notation with tests etc... It may be nice to have this deep level modify/remove method for this package.

@capellopablo this is on the master branch now, I can do a release in a bit but want some clarification from @Androlax2 if this is considered complete or not.

[stevep]

Ok @Androlax2 @capellopablo @marcelo2605 I pushed a new code to develop and master branches that builds upon the code here.

For deep nesting I changed using . to -> incase people already have . in fields names for some reason.
Also this should work when passing a closure to ModifyField. Check out the code in the GroupBuilderTest.php for examples

d7dce8b#diff-387508aa2a6461e10fda9888c2fc11a4789d12a65dfebbaabea92a0ccbffa0a7

[stevep]

I hadn't pushed a test for it yet, but you also don't need to use deep nesting either. The following will now work.

$subject = new FieldsBuilder('test');
$subject
    ->addRepeater('items')
        ->addText('headline');

$subject->getField('items')->modifyField('headline', [
    'wrapper' => [
        'width' => '77%'
    ]
]);

[Androlax2]

Ok @Androlax2 @capellopablo @marcelo2605 I pushed a new code to develop and master branches that builds upon the code here.

For deep nesting I changed using . to -> incase people already have . in fields names for some reason.
Also this should work when passing a closure to ModifyField. Check out the code in the GroupBuilderTest.php for examples

d7dce8b#diff-387508aa2a6461e10fda9888c2fc11a4789d12a65dfebbaabea92a0ccbffa0a7

Thank you! the -> is interesting yes (I preferred the dot notation to stick to conventions in several languages: Laravel etc. for example)

All that's left is to update the documentation https://github.com/StoutLogic/acf-builder/wiki/modifying-fields

[stevep]

@Androlax2 I hear ya about the dot notation. I just worry about a hypothetical user who might named a field something like image.background And even though use of modifyField is probably low, I don't want to chance breaking someone's code in a minor release. Same reason I wouldn't use a dash or an underscore as a delimiter. Arrow is used in PHP as an object accessor, so I thought it was the next logical choice.

Then I thought about making the delimiter a public static variable on FieldsBuilder but then worried if an external library is also using ACFBuilder, and a user's code has a different delimiter than the library's code, one of them will break the other.

I will get this wrapped up into a release today, along with removing deeply nested fields and update the wiki. Thanks for your help on this.

[Androlax2]

@stevep Yes, of course I understood the problem with the point notation. Maybe it would be wise in a major version to put a dot instead of the ->?

This is only my opinion of course, see what other people think, but I find it quite unnatural to use -> in a string when a . doesn't necessarily bother (thanks to the frameworks and others that use this notation).

See if it is really necessary.
It's obviously just a detail, a quibble.

[stevep]

In the interest of getting this feature finally out there I'm going to release with the arrow as the shorthand. We can revisit the dot notation later, potentially using both arrow and dot notation if and when a new major version with other breaking changes is released.

Also added is the ability to modify / remove nested flexible content fields and layouts. The wiki has been updated. Let me know if any parts in there aren't clear. Thanks again.

[capellopablo]

@Androlax2 thanks for doing this, I think this is great and will help people.

The code and tests looked good to me, I merged it.

I'm not sure what you meant by your last message though, the dot notation was present in the 2nd commit

If you are ok, I can deep the code and do something with this dot notation with tests etc... It may be nice to have this deep level modify/remove method for this package.

@capellopablo this is on the master branch now, I can do a release in a bit but want some clarification from @Androlax2 if this is considered complete or not.

Thanks so much! It's working perfect!