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

replace diffAssoc with diff so the method is removed when there are parameters in front of it #2299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TomNealBorneman
Copy link

@TomNealBorneman TomNealBorneman commented Jul 6, 2022

Fixes #2216

@tabuna
Copy link
Member

tabuna commented Jul 7, 2022

Hi @TomNealBorneman, could you add some tests to test this case?

@TomNealBorneman
Copy link
Author

TomNealBorneman commented Aug 19, 2022

When using Model bindings in your screen method (say the save function for example) you get a 404 when trying to save a new instance of a child relation:

image

image

This happens because it tries to use the method as a key for finding the model. And a model with the key 'save' does not exist. Changing the diffAssoc to diff makes it so the method is removed from the parameters regardless of its position within the collection. The following dumps (made after src\Screen\Screen.php:220) show the different results.

diff:
image

diffAssoc:
image

Dump:
image

This still does not work when you use extra url parts like http://laravel-cms.test/dashboard/users/38/test/create because then it will still use create as a key for the model binding. But in its current iteration the binding does not work at all.

This fix could cause issues if you have models with string keys that could overlap with methods, say if you have a model with the key save it would bind that instead of creating a new model as intended. But this seems like an edge case to me.

This has no effect on saving existing models like so: http://laravel-cms.test/dashboard/users/38/test/1 because the identifier is present and can be used for the binding.

edit: add clarification of parameters dumped in screenshots.

jgarivera added a commit to radical-doubting/anikultura-backend that referenced this pull request Sep 10, 2022
jgarivera added a commit to radical-doubting/anikultura-backend that referenced this pull request Sep 13, 2022
* Install prometheus exporter

* Add prometheus service provider

* Add census metrics

* Add setup agent script

* Ignore agent

* Add production dockerfile

* Add agent config

* Add install agent composer task

* Add agent worker

* Move setup agent to post install

* Add prometheus service to compose

* Run linter

* Implement collectors

* Fix prometheus network

* Add agent doc

* Add laravel scrape config

* Run lint fix

* Add gauge helpers

* Convert site insights to gauge

* Override screen handle to fix save method params

Used the workaround proposed in this [PR](orchidsoftware/platform#2299)

* Add seed allocation gauge

* Add model typehint

* Convert batch insights to gauge

* Remove redundant crop metric

* Convert crop insights to gauge

* Add Grafana dashboard export jsons

* Change values to float

* Remove unnecessary cast

* Add crop profit dashboard

* Convert farmland insights to gauge

* Run lint fix

* Add farmland dashboard

* Add estimated yield amount metric

* Convert farmer report insights to gauge

* Add farmer report dashboard

* Add site dashboard
@madyanmalfi
Copy link

Changing to diff fixes the issue for me :). If this isn't going to get merged, then please provide anther solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Method save return 404 when there is a parameter id in the url
3 participants