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

Check if field is shown on update #77

Merged
merged 2 commits into from
May 7, 2022
Merged

Check if field is shown on update #77

merged 2 commits into from
May 7, 2022

Conversation

dnwjn
Copy link
Contributor

@dnwjn dnwjn commented May 6, 2022

Hi!

I was working with this package and I was using a custom field inside a ConditionalContainer. During resource creation everything worked as expected, but during resource update I was getting errors. Turned out that that custom field was added to the update query, even though it's defined with ->exceptOnForms().

This PR adds a check to see if a field is shown on update, and will not add it if not.

Hope it's helpful!

@milewski
Copy link
Member

milewski commented May 6, 2022

Im not sure if this is the right place to add this check... if I understood correct whenever you try to update it is trying to fill fields that shouldn't be there because of ->exceptOnForms() did you try checking on this function https://github.com/dcasia/conditional-container/blob/development/src/HasConditionalContainer.php#L89 if the field is excluded?

@dnwjn
Copy link
Contributor Author

dnwjn commented May 6, 2022

@milewski I did some more digging and it turns out that inside the method you mentioned, filtering is done inside this if:

if ($controller instanceof CreationFieldController ||
            $controller instanceof UpdateFieldController) {

But it doesn't go there, since the controller is an instance of Laravel\Nova\Http\Controllers\ResourceUpdateController, so no filtering is done and all fields are returned.

Edit: If I change above to the following, it works:

if ($controller instanceof CreationFieldController ||
            $controller instanceof UpdateFieldController ||
            $controller instanceof ResourceStoreController ||
            $controller instanceof ResourceUpdateController) {

@milewski
Copy link
Member

milewski commented May 7, 2022

if ($controller instanceof CreationFieldController ||
$controller instanceof UpdateFieldController ||
$controller instanceof ResourceStoreController ||
$controller instanceof ResourceUpdateController) {

Yes I think this should be the proper way, that function should only return fields that should be updated/seen etc.. on the current request

@dnwjn
Copy link
Contributor Author

dnwjn commented May 7, 2022

if ($controller instanceof CreationFieldController ||
$controller instanceof UpdateFieldController ||
$controller instanceof ResourceStoreController ||
$controller instanceof ResourceUpdateController) {

Yes I think this should be the proper way, that function should only return fields that should be updated/seen etc.. on the current request

I have removed my earlier change and incorporated the new one!

@milewski milewski merged commit 7e2e407 into dcasia:development May 7, 2022
@dnwjn dnwjn deleted the fix/shown-on-update branch May 7, 2022 14:02
@cwray-tech
Copy link

@milewski this pr is causing an issue with uploading images. When using advanced media library, the field is being destroyed making the image unreadable when uploading. Nasty bug to track down... Set this package to 1.4.2 and bug solved.

@dnwjn
Copy link
Contributor Author

dnwjn commented May 20, 2022

@milewski this pr is causing an issue with uploading images. When using advanced media library, the field is being destroyed making the image unreadable when uploading. Nasty bug to track down... Set this package to 1.4.2 and bug solved.

@milewski I think (not sure though) that the field from Advanced Media Library should be added to the list of instanceof checks in ConditionalContainer::resolveDependencyFieldUsingRequest()?

Edit: "Not sure" because I haven't had time to test yet.

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

Successfully merging this pull request may close these issues.

None yet

3 participants