tybruffy / ACF-Reusable-Field-Group

Advanced Custom Fields Plugin to reuse Field Groups
53 stars 16 forks source link

PHP Error when using on Settings Page #2

Closed solepixel closed 9 years ago

solepixel commented 9 years ago

First of all thanks for this field group. Really hope to see this make it into the plugin.

One thing I noticed was a PHP Notice is thrown when trying to use this on a settings page: Notice: Trying to get property of non-object in [...]/wp-content/plugins/ACF-Reusable-Field-Group/acf-reusable_field_group-v5.php on line 139

This is trying to use global $post and variable doesn't exist on a settings page. I haven't looked into the code to figure out what prefix is being used to store data for settings pages, but I imagine a check needs to be added on the render_field method to support settings pages, users, widgets, comments, taxonomies, etc.

Code refrerenced here: https://gist.github.com/solepixel/c1f7377a8784a185b1c6

tybruffy commented 9 years ago

Just pushed up a new version that should fix this issue. Mind checking it out and letting me know if it solves your problem?

solepixel commented 9 years ago

Seems to work great! Thanks for getting the update so quickly. Don't forget to change the version in the plugin file.

solepixel commented 9 years ago

I might've spoke too soon. It looks like for options pages, you'll need to setup another field to override the "name" attribute of the Reusable Field Group. Otherwise, I don't believe there's a way to retrieve the actual option.

For example, have a very robust field group setup called _myfieldgroup. Then I create a settings page with a Reusable Field Group that uses _myfieldgroup. Well, if I was to create a second field on the same settings page, using the same Reusable Field Group, how would I distinguish the difference between the first _myfieldgroup and the 2nd _myfieldgroup?

I think the name of the Reusable Field Group needs to be unique per instance. Please let me know if I'm wrong and how to go about getting the value. Thanks!

tybruffy commented 9 years ago

Ah yikes. I don't think I'll have time to look at this today/tomorrow, but I should have some time to get a patch out this weekend

solepixel commented 9 years ago

No worries, thanks again for getting it fixed!

tybruffy commented 9 years ago

Hmm, Let me just lay this out so I understand. You have the following group:

my_field_group
  - my_text_field
  - my_img_field

And then you have an options page that is:

my_options_page
  - my_field_group
    - (my_text_field)
    - (my_img_field)
  - my_field_group
    - (my_text_field)
    - (my_img_field)

And you want to retrieve data for my_img_field, the issue is that you can only retrieve one of the images?

solepixel commented 9 years ago

I think I'm understanding you correctly. There's no way to identify the difference between 2 re-used field groups on the same options page. The plan is to have it setup like this:

Content Fields: Flexible Content Field (_my_flexible_field) Conditions: [Is Page]

Site Footer: Reusable Field Group (_my_flexible_field) Conditions: [Is Options Page 1]

Sidebar: Reusable Field Group (_my_flexible_field) Conditions: [Is Options Page 1]

So, from Options Page 1, retrieving _my_flexible_field option can't be distinguished between Site Footer or Sidebar.

Does that make sense?

tybruffy commented 9 years ago

Hmm, I hadn't actually considered this use case before. The impetus for making this was wanting to be able take a set of fields and insert them in to another set of fields in a new order. For example, you have a Page Heading and a Page Description field that show up above the content. But on another post type you have a Page Image field and a Page Footer Field as well. The idea for this was to be able to organize your fields appropriately, while keeping only one field group. So you could end up with:

  - Page Image
  - Page Heading  (Reusable Field Group)
  - Page Description  (Reusable Field Group)
  - Page Footer

Instead of just having the Heading/Description above/under the Image and Footer fields.

I hadn't even considered using this as a sort of pseudo repeater field. I think that functionality would be a big change, since one of the reasons for doing things the way they are now is that you aren't supposed to know if the Page Heading field is included on a page regularly, or through the Reusable Field Group. It saves the data as if it was just on the page normally.

For your situation I might just use a Flexible Content Field overall. Would something like this work?

Content Fields: Flexible Content Field (_my_flexible_field)
    Conditions: [Is Page]

Site Options Fields: (Flexible Content Field) (_my_flexible_options)
    Layout 1: Site Footer
        _my_flexible_field
    Layout 2: Site Sidebar
        _my_flexible_field

Would that work?

solepixel commented 9 years ago

While I haven't tested the setup you described, I suppose in theory it could work taking the field out of the "top level" therefore preventing the chance of having duplicates, but as far as user experience goes, I don't think it's very intuitive for the user to "Add Site Footer" layout or "Add Site Sidebar" layout within the same interface.

I will consider using this as a work around, but I think this plugin could really add a lot of possibilities if it fully supported being on options pages directly and other forms supported by ACF. Seems like the route would be to check the location of the field (like on option page vs page meta), then display an additional (optional) field for the field name (to override the default and prevent conflicts).

Thanks again for the suggestion and let me know if you come up with anything else. I have notified Elliot Condon about this plugin/functionality and he's considering possibly making it a part of the ACF Pro package.

tybruffy commented 9 years ago

Very cool! One reason I want to keep it as is right now is that I don't want to get too far away from what I consider the "ACFy" way of doing things. If you want to bring Elliot in on this discussion I'd be interested to hear what he has to say. if he can help us think of a way to do this the right way I'd be happy to take it down the road you've suggested, and even happier if he wanted to bring it into the core.

I'll close this issue for now, but if we get some other opinions on it I will reopen it.

tybruffy commented 9 years ago

Ok, so I gave some more thought to your issue, and I had been working on something else that you may find useful. It's still not exactly what you want, and it's definitely still a sort of roundabout mechanism for doing this. I've created another field for ACF called a Field Set. It's brand new and I would venture that there are more than a few bugs that you may encounter. But essentially it lets you group fields. It is not meant to be reusable like this plugin is.

There are two ways to do this, but one is a little more complicated than the other. We'll start with the simple version.

If you don't actually need to ensure that your fields are tied to the same group of fields, it's fairly easy to do this:

Site Footer Fields [Field Set]
    - First Sub Field [Text Field] 
    - Second Sub Field [Image Field] 

Sidebar Fields  [Field Set]
    - First Sub Field [Text Field] 
    - Second Sub Field [Image Field] 

In this scenario, you just have two field sets. You can duplicate the first one to get the second, but they aren't actually loading the same fields. The other way is more complex, but ensures that you are always loading fields from the same place.

Content Fields: [Field Group]
    Conditions: [Is Page]

    My Flexible Field [Flexible Content Field] (_my_flexible_field)

Site Options [Field Group]
    Conditions: [Is Options Page 1]

    Site Footer: [Field Set] (_my_site_footer)
        Content Fields [Reusable Field Group]
    Sidebar: [Field Set] (_my_sidebar)
        Content Fields [Reusable Field Group]

There's a fair bit of nesting there, but essentially you end up with: