StoutLogic / acf-builder

An Advanced Custom Field Configuration Builder
GNU General Public License v2.0
794 stars 62 forks source link

Incorrect typing/interface? #166

Open michaelw85 opened 1 year ago

michaelw85 commented 1 year ago

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

stevep commented 1 year ago

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

michaelw85 commented 1 year ago

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

stevep commented 1 year ago

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

michaelw85 commented 1 year ago

@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 😎

stevep commented 1 year ago

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.

michaelw85 commented 1 year ago

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