-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fix 3256 update document when attribute name is numeric #5723
Fix 3256 update document when attribute name is numeric #5723
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find! Let's add a test in DatabasesBase
that check this fix
What's Changed?
Test Plan
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a new test for the changes, called testNumericOnlyAttributes
, to keep the concerns separate from the other tests
@abnegate |
@abnegate |
@abnegate |
…ribute-name-is-numeric
@fogelito can you review this PR please. |
@@ -3279,7 +3279,7 @@ function updateAttribute( | |||
$permissions = $document->getPermissions() ?? []; | |||
} | |||
|
|||
$data = \array_merge($document->getArrayCopy(), $data); // Merge existing data with new data | |||
$data = $data + $document->getArrayCopy(); // Merge existing data with new data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @fanatic75 solve this issue with array_merge_recursive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, array_merge_recursive
might be worth exploring
|
||
$this->assertEquals($document['body']['1'], 'one updated'); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add two more tests, please?
one for find and second for getDocument , while trying to use the select query operator for this specific attribute?
for checking validations are passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fogelito this can be solved using + operator as well although I think we should first discuss if we want to allow attribute with numeric names or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree 👍
@faisalill I think we have a change of plans; We will not allow attributes with numeric names. |
@fanatic75 imma close this PR then. |
What does this PR do?
Replaced
array_merge
with+
operator.array_merge
appends the numeric keys from both the arrays and renumbers them starting from0
, hence giving weird errors likeUnknown Attribute: "0"
. Whereas+
operator uses the value from first array in the expression while retaining the same keys.Test Plan
issue3256.online-video-cutter.com.mp4
Related PRs and Issues
#3256
Checklist