acf-builder icon indicating copy to clipboard operation
acf-builder copied to clipboard

Incorrect typing/interface?

Open michaelw85 opened this issue 2 years ago • 6 comments

I keep seeing the interface NamedBuilder but when I use getFields() on a FieldsBuilder what I'm really getting is an array containing FieldBuilder. I'm on package version v1.12.0.

$builder = new FieldsBuilder('builder');
$builder->addText('something');
dd($builder->getFields());

image

michaelw85 avatar Jan 26 '23 09:01 michaelw85

So you're saying that the return doctype on the getFields() functions should be FieldBuilder[] ? I can't honestly say why I chose to return NamedBuilder[] here. It's not incorrect, but it's just not as accurate as possible like you said. And also probably not useful. I went back to see if there was a legacy reason but came up empty.

If it doesn't screw anything up, and it makes auto complete better, I'm down for changing it.

    /**
-    * @return NamedBuilder[]
+    * @return FieldBuilder[]
     */
    public function getFields()
    {
        return $this->getFieldManager()->getFields();
    }

Probably need to change it in FIeldManager as well

stevep avatar Jan 26 '23 21:01 stevep

Yes exactly, if you want I could have a look. I would like to contribute.

michaelw85 avatar Jan 27 '23 06:01 michaelw85

Go for it! I’d like to see the autocomplete benefits. Maybe take some screenshots before and after your changes. Thanks!

stevep avatar Jan 28 '23 20:01 stevep

@stevep I've create a PR #167 Please let me know if anything needs updating/changes. PS I noticed php 8 is not supported but we have 200+ sites running on PHP 8.1 using the acf builder without any issues 😎

michaelw85 avatar Jan 28 '23 21:01 michaelw85

200 sites wow! That's awesome. It works on my PHP 8+ instances too. I just haven't taken the time to get the unit tests / phpunit setup to be tested with PHP 8.

Responded to the PR in the PR, will follow up again soon.

stevep avatar Jan 30 '23 00:01 stevep

That's awesome. It works on my PHP 8+ instances too. I just haven't taken the time to get the unit tests / phpunit setup to be tested with PHP 8.

I just spend some time making the unit tests PHP 8 compatible and there are quite some things that have been removed. Here are the PHP unit versions + supported PHP version: https://phpunit.de/supported-versions.html

What has been changed: \PHPUnit_Framework_TestCase Needs to be replaced with namespaced TestCase. assertArraySubset has been removed see ticket, I've added a package re-adding it via trait. prophesize is not included in phpunit anymore, as of phpunit 9 you will have to install it yourself and use a trait, which I did but this library supports PHP 7.3 as a minimum. assertInternalType Is deprecated expectedException Should be written with an assertion

Here's the PR: https://github.com/StoutLogic/acf-builder/pull/168

michaelw85 avatar Jan 30 '23 08:01 michaelw85