fix: [Table] field named data will produce bugged output
#8054
Conversation
kenjis
added
bug
|
@ping-yee Can you add test code? |
kenjis
changed the title
data will produce bugged output problem.data will produce bugged output
|
@kenjis Done. |
| @@ -265,7 +265,7 @@ protected function _prepArgs(array $args) | |||
| // If there is no $args[0], skip this and treat as an associative array | |||
| // This can happen if there is only a single key, for example this is passed to table->generate | |||
| // array(array('foo'=>'bar')) | |||
| if (isset($args[0]) && count($args) === 1 && is_array($args[0]) && ! isset($args[0]['data'])) { | |||
There was a problem hiding this comment.
It will cause the setHeading function to be executed incorrectly when the heading field contain named data.
There was a problem hiding this comment.
The && ! isset($args[0]['data']) came from this commit: bcit-ci/CodeIgniter@f0b69a85
Are you absolutely sure that just removing it does not cause another issue?
Frankly, I don't get the code yet.
There was a problem hiding this comment.
I am not sure but the all of the test cases get passed.
And I also don't get where will use this condition and the data processing of the array field contains data so far.
Or we can add one more process to check that whether the heading array has been handled correctly when the table is being generated.
There was a problem hiding this comment.
the all of the test cases get passed.
Yes. But we cannot see the coverage in Coveralls.
coverage/coveralls — Target branch is ahead of PR base. Rebase the PR to fix.
Can you rebase the PR branch?
There was a problem hiding this comment.
Can you rebase the PR branch?
Done.
There was a problem hiding this comment.
Based on my usage of the table class (quite extensive) I have not noticed any issues thus far with the patch applied.
kenjis
added
the
help wanted
|
If anyone has a good understanding of the Table's code, please review this PR. |
ping-yee
force-pushed
the
231018_generate_table
branch
from
7500d18
to
39903e6
Compare
|
This bug has been introduced in: |
|
The Table class is well-tested, and I don't see the reason to exclude |
Description
Fixes #8051
Checklist: