Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect typing/interface? #166

Open
michaelw85 opened this issue Jan 26, 2023 · 6 comments
Open

Incorrect typing/interface? #166

michaelw85 opened this issue Jan 26, 2023 · 6 comments

Comments

@michaelw85
Copy link
Contributor

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
Copy link
Member

stevep commented Jan 26, 2023

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
Copy link
Contributor Author

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

@stevep
Copy link
Member

stevep commented Jan 28, 2023

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

michaelw85 added a commit to michaelw85/acf-builder that referenced this issue Jan 28, 2023
@michaelw85
Copy link
Contributor Author

@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
Copy link
Member

stevep commented Jan 30, 2023

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
Copy link
Contributor Author

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: #168

stevep added a commit that referenced this issue Feb 7, 2023
#166 Replace NamedBuilder interface with FieldBuilder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants