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

Allow other fields to use subfields attribute (not just checklist_dependency) #4069

Merged

Conversation

tabacitu
Copy link
Member

@tabacitu tabacitu commented Jan 10, 2022

WHY

Fixes #4065

BEFORE - What was wrong? What was happening before this PR?

We couldn't use subfields for repeatable and relationship because they were used by checklist_dependency in a different way:

  • in checklist_dependency, the subfields were treated as regular fields by the CRUD; the two subfields were saved creating & updating, NOT the main field;
  • in repeatable and relationship we want the main field to be saved, not the subfields (actually, the subfields will be part of the main field because they have the same name as it, just add stuff using bracket notation - eg. testimonial[name] and category[body]);

AFTER - What is happening after this PR?

  • subfields can be used the way repeatable and relationship want them (as fields with bracket names)
  • subfields can STILL be used by checklist_dependency, because Backpack treats them as regular fields if the field NAME is an array;

HOW

How did you achieve that, in technical terms?

In short:

  • if $field['name'] is a string, subfields are considered inconsequential;
  • if field['name'] is an array, subfields are considered important, are checked for relationships;

This is probably not a long-term solution, but it's the only way I've found to mix this in a non-breaking way, without introducing a flag for checklist_dependency or anything.

It has the added benefit that what we coded for date_range helps in this respect. Only date_range and checklist_dependency use arrays as field names, and they both need the same functionality: save both inputs in the name array. In addition to the date_range field, what checklist_dependency needs extra is for the subfields to be checked for relationships, that's it.

Is it a breaking change or non-breaking change?

Non-breaking.

How can we test the before & after?

Before - inside the Users CRUD check that roles & permissions work.
After - inside the Users CRUD check that roles & permissions work.


Sidenotes:

@tabacitu tabacitu self-assigned this Jan 10, 2022
…o-not-use-subfields-attribute-only-for-checklist-dependency-field
@tabacitu tabacitu requested a review from pxpm January 11, 2022 08:24
@tabacitu tabacitu removed their assignment Jan 11, 2022
@tabacitu tabacitu changed the title Do not use subfields attribute only for checklist dependency field Allow other fields to use subfields attribute (not just checklist_dependency) Jan 11, 2022
@tabacitu
Copy link
Member Author

@pxpm this is ready to merge IMHO. Please take a look, review and test. Let me know if I can merge OR you pick up anything wrong with it.

…o-not-use-subfields-attribute-only-for-checklist-dependency-field
@pxpm
Copy link
Contributor

pxpm commented Jan 13, 2022

This is pending #4078 (comment) comment resolution.

@tabacitu tabacitu mentioned this pull request Jan 14, 2022
@tabacitu
Copy link
Member Author

Merged PR #4078 into this one, removing fields in favor of subfields in relationship. Note that repeatable still supports both fields and subfields (for backwards-compatibility).

@tabacitu tabacitu merged commit 7ce65db into 4.2 Jan 14, 2022
@tabacitu tabacitu deleted the do-not-use-subfields-attribute-only-for-checklist-dependency-field branch January 14, 2022 06:34
@tabacitu tabacitu mentioned this pull request Feb 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants