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

Make select_multiple and select2_multiple fields works as subfields and dot.notation.fields (through calling create() recursively) #4127

Merged
merged 70 commits into from
Feb 2, 2022

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Jan 30, 2022

WHY

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

Nested relations were not properly saved nor displayed.

AFTER - What is happening after this PR?

They save, they display, they work.

HOW

How did you achieve that, in technical terms?

This a 100% replacement for #4115

Everything is splitted in small commits so we can move them out to new PR's if needed, but they are all related to this PR .. so!

Creating of entries and relations are done recursivelly, so technically any chained relations would work no matter how long the chain, and all of them have the same way of getting direct inputs (parsing) and relations.

Our tests already provide tests for "repeatable inside repeatable", but due to interface issues they are not supported atm just there to show off our relation creations ability. 🥳

Is it a breaking change or non-breaking change?

Non breaking in 4.2

How can we test the before & after?

I've created Laravel-Backpack/demo#348 so this branch can be properly tested, it enables the fake fields and removes the unsuported relations (repeatables inside repeatables).

From my testings:

  • Monster works.
  • Cave works.
  • Story works.
  • Fake fields works.
  • Tests are passing.

I hope this time is for good after the usual rounds of code review, I think we got this to a pretty working/predictable state.

One of the biggest issues to make all this work together is the fact that we support dot.notation.relations. If we didn't support them this PR would be clean as cristal water. But still, I think it's pretty much intuitive.

Let me know if I let something slip. We talk better about it tomorrow, I want to refactor some variable_names etc etc (in a separate commit).

Pedro

Pedro

pxpm added 30 commits January 26, 2022 10:10
@tabacitu
Copy link
Member

tabacitu commented Feb 2, 2022

EXCELEEEENT!!! Good job @pxpm , I could now test, and can confirm select_multiple and select2_multiple are working now, as subfields 🎉🎉🎉

  • Monster still works 🎉
  • Cave works 🎉 (select_multiple and select2_multiple too)
  • Story works 🎉 (select_multiple and select2_multiple too)

Now let's talk about how it works, or why it works. Let's polish this PR a bit, to get it merged to 4.2. Then we can make separate PRs for the other stuff (like you did for the fake fields 👏👏👏).

@tabacitu
Copy link
Member

tabacitu commented Feb 2, 2022

Ok so I've given this a hard look and (good news!) we're 10/17 files approved. Please take a look at my comment here, if something like that is possible, it should resolve 5 more files 🙀

Then for the 2 remaining files, I think we can have a quick talk, see what (if anything) we do about my concerns.

Overall... I feel like we're very very close to having this merged.

@tabacitu tabacitu merged commit 708535a into 4.2 Feb 2, 2022
@tabacitu tabacitu deleted the use-create-recursively branch February 2, 2022 15:57
@tabacitu
Copy link
Member

tabacitu commented Feb 2, 2022

Screenshot 2022-02-02 at 17 58 02

OMGOMGOMGOMGOMGOM!!!! 😱🎉🎉🎉🎉

Thank you so much for your patience with this @pxpm 🙏

This was referenced Feb 2, 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.

3 participants