Skip to content

Fix bug where custom fields are not properly merged#199

Merged
Gummibeer merged 2 commits intofenos:masterfrom
sudomabider:fix-merging-custom-fields-bug
Aug 25, 2016
Merged

Fix bug where custom fields are not properly merged#199
Gummibeer merged 2 commits intofenos:masterfrom
sudomabider:fix-merging-custom-fields-bug

Conversation

@sudomabider
Copy link
Contributor

@sudomabider sudomabider commented Aug 18, 2016

I was experiencing 2 issues:

  1. HTTP requests and console inputs are behaving differently for this particular scenario: custom fields get ignored for http request but are merged fine in the console. I think what happens is: Config is merged in the service provider at model resolution. But then merged again in the model constructor in a different way. It is also why the test is not catching the bug. The bug currently only exists for http requests (I didn't have time to dig deep enough to explain why but I think the model resolution callback has something to do with it).
  2. In the Notificatoin model, getCustomFillableFields() flattens the additional fields array from the config file which forces it to be a numeric array. And then mergeFillable() uses array addition (+) which ignores the custom fields because the numeric keys already exist in the left-hand array.

Veo Chen added 2 commits August 18, 2016 15:57
properly merge custom fields to Notification's fillable data
@Gummibeer Gummibeer added the bug label Aug 18, 2016
@Gummibeer Gummibeer merged commit ea06306 into fenos:master Aug 25, 2016
@Gummibeer
Copy link
Collaborator

Hey, sorry for the long delay and thanks for your help! :)

@sudomabider sudomabider deleted the fix-merging-custom-fields-bug branch August 25, 2016 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants