Conversation
…R_CLASSNAME, Type::TYPE_IDENTIFIER_METHOD, etc constants
Element::getHtml() - no longer indent by default (affecting elements with white-space:pre
Remove last remnants of `isTraverseOnly`
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
* 3.6: Laravel/ServiceProvider::$logDb undefined prop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/Teams/Elements/Table.php (1)
27-36:⚠️ Potential issue | 🟠 MajorConstructor type hint breaks Traversable inputs (use
iterable).
asRows()and the docblock accept Traversable, but the newarraytype hint rejects it, causing a TypeError for iterator-based callers. This is a behavioral regression.✅ Suggested fix
- public function __construct(array $rows = array()) + public function __construct(iterable $rows = array())tests/Debug/Plugin/Method/TraceTest.php (1)
128-171:⚠️ Potential issue | 🟠 MajorFix HTML trace expectations to use table rows from
getValues()
$logEntry['args'][0]is now a table abstraction; numeric indexing ($trace[$i - 1]) is legacy and will yield null/incorrect data. Also, the index column is already present in the row data, so prefixing it duplicates the index. Build expectations fromgetValues()['rows']and usechildrenwhen present.Proposed fix
- 'html' => function ($output, LogEntry $logEntry) { - $trace = $logEntry['args'][0]; + 'html' => function ($output, LogEntry $logEntry) { + $traceValues = $logEntry['args'][0]->getValues(); self::assertStringContainsString('<caption>trace</caption>' . "\n" . '<thead>' . "\n" . '<tr>' . "\n" . '<th class="t_string" scope="col"></th>' . "\n" . '<th class="t_string" scope="col">file</th>' . "\n" . '<th class="t_string" scope="col">line</th>' . "\n" . '<th class="t_string" scope="col">function</th>' . "\n" . '</tr>' . "\n" . '</thead>', self::normalizeString($output)); @@ - for ($i = 1; $i < $count; $i++) { + for ($i = 1; $i < $count; $i++) { // build expected values - - $valuesExpect = \array_merge( - [(string) ($i - 1)], - \array_values($trace[$i - 1]) - ); + $row = $traceValues['rows'][$i - 1]; + $rowValues = isset($row['children']) ? $row['children'] : $row; + $valuesExpect = \array_values($rowValues); + $valuesExpect[0] = (string) $valuesExpect[0]; $valuesExpect[1] = $this->debug->getDump('html')->valDumper->dump($valuesExpect[1], array('tagName' => null)); $valuesExpect[2] = $valuesExpect[2] === null ? 'null' : (string) $valuesExpect[2]; $valuesExpect[3] = $this->debug->getDump('html')->valDumper->dump($valuesExpect[3], array('tagName' => null));src/Debug/Plugin/Method/Trace.php (1)
120-139:⚠️ Potential issue | 🔴 CriticalFix argument mismatch when calling
getTraceFinish.
Line 138 passes$meta, butgetTraceFinishonly accepts one parameter (Line 184), which raises anArgumentCountErrorin PHP 8+. Remove the extra argument.🐛 Suggested fix
- return $this->getTraceFinish($trace, $meta); + return $this->getTraceFinish($trace);src/Debug/js_src/FileLinks.js (1)
115-133:⚠️ Potential issue | 🟡 MinorFix remove path to avoid leaving extra header/colspan and creating links.
Whenremoveis true and no links exist, this now adds a header and link column. When removing existing links, the header and context-row colspan aren’t reverted, leaving the table misaligned.🔧 Proposed fix
function createFileLinksTrace ($entry, remove) { var isUpdate = $entry.find('.file-link').length > 0 - if (!isUpdate) { - $entry.find('table thead tr > *:last-child').after('<th></th>') - } else if (remove) { - $entry.find('table tr:not(.context) > *:last-child').remove() - return - } + if (remove) { + if (isUpdate) { + $entry.find('table thead tr > *:last-child').remove() + $entry.find('table tbody tr:not(.context) > *:last-child').remove() + $entry.find('table tbody tr.context').each(function () { + var $td0 = $(this).find('> td').eq(0) + var colspan = parseInt($td0.attr('colspan'), 10) + if (!isNaN(colspan) && colspan > 1) { + $td0.attr('colspan', colspan - 1) + } + }) + } + return + } + if (!isUpdate) { + $entry.find('table thead tr > *:last-child').after('<th></th>') + } $entry.find('table tbody tr:not(.context)').each(function () { createFileLinksTraceProcessTr($(this), isUpdate) }) if (isUpdate) { return; }src/Debug/Abstraction/Abstracter.php (1)
62-62:⚠️ Potential issue | 🟡 MinorPHPDoc syntax error: malformed type annotation.
The type annotation
array>string,mixed>should bearray<string,mixed>.- /** `@var` array>string,mixed> */ + /** `@var` array<string,mixed> */
🤖 Fix all issues with AI agents
In `@src/Container/ObjectBuilder.php`:
- Around line 82-96: The build method currently ignores the $useContainer flag
by always checking $this->container->has($classname) first; change the logic so
you only consult the container when $useContainer is true (e.g., guard the
container->has and container->get calls with $useContainer) and likewise only
call $this->container->offsetSet($classname, $return) when $useContainer is
true; update the build(...) implementation (referencing build, $useContainer,
$this->container->has, $this->container->get, and $this->container->offsetSet)
so that setting and returning container instances are skipped when $useContainer
is false.
In `@src/Debug/Abstraction/Abstracter.php`:
- Line 20: The unconditional top-level import "use SensitiveParameterValue" and
the runtime "instanceof SensitiveParameterValue" check in Abstracter.php will
crash on PHP <8.2; remove the unconditional use and change the runtime check to
be guarded (e.g., if (defined('PHP_VERSION_ID') && PHP_VERSION_ID >= 80200) { /*
instanceof check */ } or use if (class_exists('SensitiveParameterValue')) { /*
instanceof check using fully-qualified class name */ }), or alternatively update
composer.json to require PHP >= 8.2; ensure all references in the Abstracter
class to SensitiveParameterValue use the guarded path or class_exists guard so
older PHP versions never try to import or evaluate that symbol.
In `@src/Debug/Abstraction/Object/Subscriber.php`:
- Around line 301-314: The onStartTable method can leave
tableCellValueAbstracter subscribed and isAbstractingTable true if
jsonSerialize() or crate() throws; wrap the core work (subscribe, values
assignment, crate call, setting values['type'] and abs['unstructuredValue']) in
a try/finally so you always call
$debug->eventManager->unsubscribe(Debug::EVENT_OBJ_ABSTRACT_END, [$this,
'tableCellValueAbstracter']) and set $this->isAbstractingTable = false in the
finally block; ensure unsubscribe and flag-reset occur even on exceptions so
subsequent abstractions are not affected.
In `@src/Debug/Abstraction/Type.php`:
- Around line 190-200: The code uses "instanceof UnitEnum" which fatal-errors on
PHP <8.1; update the check to be guarded so older PHP versions skip it (for
example replace the raw instanceof UnitEnum check with a conditional that first
verifies the symbol exists, e.g., class_exists('UnitEnum') or PHP_VERSION_ID >=
80100) so the branch that sets TYPE_OBJ_ENUM only runs when UnitEnum is
available; modify the elseif that references UnitEnum in the method returning
[self::TYPE_OBJECT, $typeMore] (and keep using TYPE_OBJ_ENUM / TYPE_OBJECT
unchanged) or alternatively bump the composer platform requirement to PHP 8.1+
if you prefer to drop backwards compatibility.
In `@src/Debug/Dump/Html/Table.php`:
- Around line 151-156: In buildContextRow(TableRow $row, $expanded) remove the
invalid second argument to addClass: call $row->addClass('expanded') only when
$expanded is truthy (i.e., wrap the addClass call in the existing if ($expanded)
block) and leave the subsequent $row->setAttrib('data-toggle', 'next') unchanged
so no extra argument is passed to Element::addClass.
In `@src/Debug/Dump/TextAnsi/Value.php`:
- Around line 336-352: In dumpTable(Abstraction $abs) avoid calling
\bdk\Table\Utility::asArray() when the BdkTable has no header: first construct
$table = new BdkTable($abs->getValues()), check $table->getHeader() for null and
if null return $this->dumpArray($abs->getValues()) (or
$this->dumpArray($table->getValues())) as a fallback; only call
\bdk\Table\Utility::asArray($table) and proceed with caption handling/dumping
when getHeader() is non-null so Utility::asArray() never receives a table
without a header.
In `@src/Debug/Framework/WordPress/Shortcodes.php`:
- Around line 99-105: The code is concatenating Abstraction objects returned by
$cells[2]->getValue() (e.g., $links and similarly $method) which calls
__toString() and drops metadata; instead, extract the raw string/value from the
Abstraction, perform the concatenation to build the HTML fragment, then re-wrap
the resulting string with crateWithVals() (preserving flags like
addQuotes/sanitize) before calling $cells[2]->setValue(...); apply the same
pattern for the other occurrence around $method so metadata is retained when
calling setValue() and buildInfoRow().
In `@src/Debug/LogEntry.php`:
- Around line 231-236: The code uses getMeta('detectFiles', false) which forces
a false default and overrides global config; change it so getMeta('detectFiles')
(or getMeta('detectFiles', null)) is used and only call
subject->abstracter->abstractString->setCfg('detectFiles', $detectFiles) when
the meta is not null, and only populate $cfgRestore['detectFiles'] when setCfg
returned a different previous value; update the DetectFiles handling around the
variables $detectFiles, $detectFilesWas and $cfgRestore to preserve global
behavior when meta is absent.
In `@src/Table/Element.php`:
- Around line 466-471: The unserialize method currently calls
\unserialize($data) which allows object instantiation from untrusted input;
change it to use the allowed_classes option to block object injection (e.g. call
\unserialize($data, ['allowed_classes' => false])) and keep the existing check
that only arrays are passed to __unserialize; for PHP versions older than 7
(PHP_VERSION_ID < 70000) fall back to the current \unserialize($data) behavior
behind a version check so compatibility with PHP 5.x is preserved, referencing
the unserialize method and the __unserialize call to locate the change.
In `@src/Table/TableRow.php`:
- Around line 76-84: The setChildren method currently treats any Element as
acceptable; change the instanceof check to require TableCell so only TableCell
instances are preserved: in TableRow::setChildren, map $children and replace the
conditional ($child instanceof Element) with ($child instanceof TableCell),
otherwise construct new TableCell($child), then call
parent::setChildren($children) as before to ensure rows only contain TableCell
objects.
In `@src/Table/Utility.php`:
- Around line 34-40: self::$tempData assignment calls
$table->getHeader()->getChildren() without checking for null, causing a fatal
error when getHeader() returns null; update the 'headerVals' computation in the
self::$tempData block to first check $table->getHeader() (or retrieve
$table->getHeader() into a variable) and only call ->getChildren() when it's not
null, otherwise use an empty array for headerVals, leaving the array_map logic
(and the 'columnsMeta' and 'rowKey' entries) unchanged so behavior is preserved
when a header exists.
🟡 Minor comments (11)
src/Debug/Utility/Profile.php-38-41 (1)
38-41:⚠️ Potential issue | 🟡 MinorBC break confirmed: Constructor now strictly requires array values.
The type narrowing at line 40 from untyped to
arraydoes break callers passing null or other non-array values, though the defensive cast at line 42 suggests this was intentional. All internal call sites in the repository use the default parameter and are unaffected. External library users who previously passed non-array values will receive aTypeError.If this is a deliberate API tightening, ensure the version bump and migration guide reflect the breaking change.
src/Debug/Plugin/Method/Trace.php-253-265 (1)
253-265:⚠️ Potential issue | 🟡 MinorGuard against non‑method frames. Line 258 assumes
Class->methodformat but traces containClass::staticMethod,{closure}, and fully qualified function names. Whenpreg_matchfails to match these patterns, accessing$matches['classname']triggers an undefined index notice. Add validation before accessing the array.💡 Suggested fix
private function removeMinInternal(array $trace) { $count = \count($trace); $internalClasses = [__CLASS__, 'bdk\PubSub\Manager']; for ($i = 3; $i < $count; $i++) { - \preg_match('/^(?P<classname>.+)->(?P<function>.+)$/', $trace[$i]['function'], $matches); - $isInternal = \in_array($matches['classname'], $internalClasses, true) || $matches['function'] === 'publishBubbleEvent'; + if (!isset($trace[$i]['function']) + || \preg_match('/^(?P<classname>.+)->(?P<function>.+)$/', $trace[$i]['function'], $matches) !== 1 + ) { + break; + } + $isInternal = \in_array($matches['classname'], $internalClasses, true) + || $matches['function'] === 'publishBubbleEvent'; if ($isInternal === false) { break; } } return \array_slice($trace, $i); }src/Debug/Route/Text.php-28-28 (1)
28-28:⚠️ Potential issue | 🟡 MinorAvoid overwriting a preconfigured dumper.
If callers set a custom dumper before route construction, this now unconditionally replaces it. Consider guarding the assignment.
Suggested guard
- $this->dumper = $debug->getDump('text'); + if (!$this->dumper) { + $this->dumper = $debug->getDump('text'); + }tests/Table/TableCellTest.php-593-600 (1)
593-600:⚠️ Potential issue | 🟡 MinorDrop unused
$html1variable.PHPMD flagged
Line 594as unused; removing the assignment keeps the test clean.🛠️ Suggested fix
- $html1 = $cell->getHtml(); + $cell->getHtml();tests/Table/TableCellTest.php-31-41 (1)
31-41:⚠️ Potential issue | 🟡 MinorCapture the original val dumper before overriding.
Currently you call
TableCell::setValDumper(...)before capturing the original value, so tearDown restores the default instead of the prior global state. This can leak between tests.🛠️ Suggested fix
- TableCell::setValDumper(['bdk\Table\TableCell', 'valDumper']); - // Store original dumper - $reflection = new \ReflectionClass(self::CLASS_TABLE_CELL); + // Store original dumper + $reflection = new \ReflectionClass(self::CLASS_TABLE_CELL); $property = $reflection->getProperty('valDumper'); if (PHP_VERSION_ID < 80100) { $property->setAccessible(true); } self::$originalValDumper = $property->getValue(); + TableCell::setValDumper(['bdk\Table\TableCell', 'valDumper']);src/Debug/js/Debug.js-783-792 (1)
783-792:⚠️ Potential issue | 🟡 MinorGuard against missing
colspanin context rows.If a context row lacks a
colspanattribute,parseIntreturnsNaNand the attribute becomesNaN. A safe default avoids layout glitches.🛠️ Suggested fix
- var $td0 = $(this).find('> td').eq(0); - $td0.attr('colspan', parseInt($td0.attr('colspan'), 10) + 1); + var $td0 = $(this).find('> td').eq(0); + var colspan = parseInt($td0.attr('colspan'), 10) || 1; + $td0.attr('colspan', colspan + 1);src/Table/Utility.php-92-114 (1)
92-114:⚠️ Potential issue | 🟡 MinorMissing null safety for columnsMeta array access.
Line 94 accesses
self::$tempData['columnsMeta'][$iCell]['key']without checking ifcolumnsMetaor the specific index exists. This could cause undefined index errors if table metadata is incomplete.🛡️ Proposed fix to add defensive check
private static function getCellKeyValue(TableCell $tableCell, $iCell) { + $columnsMeta = self::$tempData['columnsMeta'] ?? array(); + $columnMeta = $columnsMeta[$iCell] ?? array(); - $key = self::$tempData['columnsMeta'][$iCell]['key']; + $key = $columnMeta['key'] ?? null; $value = $tableCell->getValue();src/Table/Table.php-92-117 (1)
92-117:⚠️ Potential issue | 🟡 MinorGuard against empty thead/tfoot to avoid undefined offsets.
Lines 98 and 114: If a thead/tfoot Element exists but has zero rows (e.g., created via
new Element('thead', [])and passed tosetChildren()), accessing$rows[0]will trigger an undefined array offset. Return null when the rows array is empty.🧩 Suggested guard for empty header/footer rows
$rows = $this->tfoot->getChildren(); + if (!$rows) { + return null; + } return \count($rows) > 1 ? $rows : $rows[0];$rows = $this->thead->getChildren(); + if (!$rows) { + return null; + } return \count($rows) > 1 ? $rows : $rows[0];src/Table/Element.php-22-25 (1)
22-25:⚠️ Potential issue | 🟡 MinorFix malformed PHPDoc generic for
$cfg.The annotation
array>string,mixed>is invalid and can break static analysis tools; usearray<string,mixed>.✏️ Suggested fix
- /** `@var` array>string,mixed> */ + /** `@var` array<string,mixed> */src/Table/Element.php-158-165 (1)
158-165:⚠️ Potential issue | 🟡 MinorNormalize
getIndex()when not found.
array_searchcan returnfalse, which can be mistaken for index0by callers; returnnullexplicitly.✅ Suggested fix
- return \array_search($this, $siblings, true); + $index = \array_search($this, $siblings, true); + return $index === false ? null : $index;src/Debug/Dump/Html/Table.php-158-166 (1)
158-166:⚠️ Potential issue | 🟡 MinorGuard against missing context-value cell.
Directly accessing
$row->getCells()[2]will emit notices if the row has fewer cells. Add a defensive fallback.🛡️ Suggested fix
- $contextHtml = $this->helper->buildContext($row->getMeta('context'), $row->getCells()[2]->getValue()) + $cells = $row->getCells(); + $contextValue = isset($cells[2]) ? $cells[2]->getValue() : null; + $contextHtml = $this->helper->buildContext($row->getMeta('context'), $contextValue) . $this->buildContextArguments($row->getMeta('args'));
🧹 Nitpick comments (18)
src/Promise/AbstractResolvedPromise.php (1)
22-22: Consider adding type hints to the constructor signature.Since this PR includes typehint updates as part of coding-standard cleanups, consider adding actual type declarations to match the documented types.
🔧 Suggested enhancement with type hints
- public function __construct($state, $result) + public function __construct(string $state, $result)Note:
$resultcan remain untyped (equivalent tomixedin PHP 7.x) or be explicitly typed asmixedin PHP 8.0+.tests/Debug/Collector/DoctrineMiddlewareTest.php (1)
173-175: Remove redundant$colClassreassignment.This assignment duplicates lines 93-95 exactly. Since
$haveParamTypehasn't changed and$colClasswas already set, this can be removed.♻️ Proposed fix
- $colClass = $haveParamType - ? ' <span class="classname"><span class="namespace">Doctrine\DBAL\</span>ParameterType</span>' - : ''; $select2expect = <<<EODtests/Debug/Utility/SerializeLogTest.php (1)
110-110: Consider documenting why this test is disabled.Renaming
testBackwardsImportCompatibilitytot_estBackwardsImportCompatibilityeffectively disables this test by breaking PHPUnit's naming convention. While this is a valid technique, it lacks visibility compared to using$this->markTestSkipped()or the@skipannotation.Consider adding a comment explaining the reason for disabling this test, or use a more explicit skipping mechanism:
💡 Suggested alternatives
Option 1 - Add explanatory comment:
- public function t_estBackwardsImportCompatibility($dataFilepath) + // TODO: Re-enable once [reason] is resolved + public function t_estBackwardsImportCompatibility($dataFilepath)Option 2 - Use explicit skip:
- public function t_estBackwardsImportCompatibility($dataFilepath) + public function testBackwardsImportCompatibility($dataFilepath) { + $this->markTestSkipped('Temporarily disabled: [reason]'); $serialized = \file_get_contents($dataFilepath);tests/Debug/data/PropertyHooks.php (1)
256-256: Consider removing the commented-out line entirely.If
traverseValuesis no longer part of the data structure (as indicated by similar changes across other test fixtures), the commented line should be removed rather than left as dead code.♻️ Suggested fix
'sort' => 'inheritance visibility name', 'stringified' => null, - // 'traverseValues' => array(), 'type' => Type::TYPE_OBJECT,tests/Debug/Route/ChromeLoggerTest.php (1)
35-35: Inconsistent route access pattern within the same test file.Line 35 now uses
getRoute('chromeLogger'), but lines 66 and 129 still use the magic property$this->debug->routeChromeLogger. Consider updating all occurrences for consistency.♻️ Suggested fix for consistency
Update lines 66 and 129 to use the same getter pattern:
- $this->debug->routeChromeLogger->setCfg('group', false); + $this->debug->getRoute('chromeLogger')->setCfg('group', false);src/Debug/Dump/TextAnsi/TextAnsiObject.php (1)
115-117: Inconsistent use of ANSI escape codes.The
dumpPropertiesmethod still uses inline escape sequences"\e[4m"and"\e[24m"instead of the newly definedself::ANSI_UNDERLINEandself::ANSI_NO_UNDERLINEconstants used indumpMethods.♻️ Suggested refactor for consistency
$header = \count($abs['properties']) > 0 - ? "\e[4m" . $this->debug->i18n->trans('object.properties') . ':' . "\e[24m" . "\n" + ? self::ANSI_UNDERLINE . $this->debug->i18n->trans('object.properties') . ':' . self::ANSI_NO_UNDERLINE . "\n" : $this->debug->i18n->trans('object.properties.none') . "\n";src/Debug/Plugin/Method/Table.php (1)
136-161: Unused$isRowparameter - verify callback interface compliance.Static analysis flags
$isRowas unused. If this is part of theTableFactory::getValInfocallback signature, consider adding a brief comment explaining why it's present but unused, or prefix with underscore ($_isRow) to indicate intentional non-use.💡 Suggested documentation
/** * "getValInfo" callback used by TableFactory * * `@param` mixed $value Value to get type of - * `@param` bool $isRow Does value represent a row + * `@param` bool $isRow Does value represent a row (unused, required by callback signature) * * `@return` array<string,mixed> */ - public function getValInfo($value, $isRow = false) + public function getValInfo($value, $isRow = false) // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParametersrc/Debug/Route/Firephp.php (1)
220-243: Add a defensive guard for unexpected tabular args.
methodTabularassumesargs[0]is an object withgetValues(). If a malformed/empty table entry slips through (custom callers, edge-case trace), this will fatal and abort header output. Consider short‑circuiting when the arg isn’t table-like.🛡️ Suggested guard
- $data = $logEntry['args'][0]->getValues(); + $tableAbs = $logEntry['args'][0] ?? null; + if (!$tableAbs || !\is_object($tableAbs) || !\method_exists($tableAbs, 'getValues')) { + return null; + } + $data = $tableAbs->getValues();tests/Table/TableCellTest.php (1)
329-345: Avoid leaving commented-out tests.Consider either enabling the
DateTimeImmutabletest or removing the commented block to keep the suite clean.src/Table/TableRow.php (2)
14-20: RedundanttagNamedeclaration in both$defaultsand$tagName.Consider removing the redundant
$tagNameproperty declaration if the parent class can initialize it from$defaults. However, this may be intentional for direct property access performance.
27-34: Constructor heuristic for distinguishing children from properties may be fragile.The condition
ArrayUtil::isList($children) || \reset($children) instanceof Elementassumes that if the array is associative and the first element is not an Element, it must be properties. An empty array would passisList()but the semantics might be unclear.Consider adding a docblock example clarifying expected input formats for better maintainability.
src/Debug/Framework/Laravel/ServiceProvider.php (1)
141-158: Consider handling potential ReflectionException.
new ReflectionClass($class)on line 145 can throwReflectionExceptionif the class doesn't exist or isn't loadable. While model classes from Eloquent events should always exist, defensive error handling would improve robustness.🛡️ Proposed fix to add try-catch
private function buildModelCountTable() { $modelCounts = array(); foreach ($this->modelCounts as $class => $count) { + try { + $ref = new ReflectionClass($class); + $fileName = $ref->getFileName(); + } catch (\ReflectionException $e) { + $fileName = null; + } - $ref = new ReflectionClass($class); $modelCounts[] = array( 'model' => $this->debug->abstracter->crateWithVals($class, array( 'attribs' => array( - 'data-file' => $ref->getFileName(), + 'data-file' => $fileName, ),src/Table/Utility.php (3)
15-17: Static state properties can cause issues with reentrant calls.Using static
$optionsand$tempDataproperties means callingasArray()on a different table while one is being processed would corrupt the state. Consider using instance methods or passing state through parameters.
66-71: Remove commented-out code block.The commented-out KEY_INDEX handling code should be removed or, if intentionally kept for reference, should include a comment explaining why it's preserved.
77-80: Variable$cellInfoaccessed outside its declaration scope.The variable
$cellInfois declared inside the foreach loop but accessed after the loop exits. While this works in PHP, it's a code smell. Consider restructuring to make the data flow clearer.♻️ Proposed refactor for clarity
private static function rowAsArray(TableRow $tableRow, $iRow) { $row = array(); + $lastCellInfo = null; self::$tempData['rowKey'] = $iRow; foreach ($tableRow->getCells() as $iCell => $tableCell) { $cellInfo = self::getCellKeyValue($tableCell, $iCell); + $lastCellInfo = $cellInfo; $key = $cellInfo['key']; $value = $cellInfo['value']; if ($key === null) { continue; } $row[$key] = $value; } - if (\count($row) === 1 && $cellInfo['isScalar'] && self::$options['forceArray'] === false) { + if (\count($row) === 1 && $lastCellInfo && $lastCellInfo['isScalar'] && self::$options['forceArray'] === false) { // this scalar column is the only column other than the index - $row = $row[$key]; + $row = $row[$lastCellInfo['key']]; } return $row; }tests/Table/TableRowTest.php (1)
527-539: Static analysis warning:$rowvariable appears unused but serves a purpose.The
$rowvariable establishes the parent-child relationship needed forgetIndex()to work. Consider adding a brief comment or using the variable explicitly to clarify intent and silence the warning.🔧 Optional: Suppress or clarify the unused variable
public function testGetIndex() { $cell1 = new TableCell('A'); $cell2 = new TableCell('B'); $cell3 = new TableCell('C'); $row = new TableRow([$cell1, $cell2, $cell3]); + self::assertSame('tr', $row->getTagName()); // Verify row setup and use $row - // TableRow's children are the cells self::assertSame(0, $cell1->getIndex()); self::assertSame(1, $cell2->getIndex()); self::assertSame(2, $cell3->getIndex()); }tests/Debug/Plugin/Method/TableTest.php (2)
44-67: Commented-out test code should be addressed.The
testColKeysmethod is fully commented out. If the test is no longer relevant due to the refactor, it should be removed entirely. If it needs to be updated, a TODO comment explaining the plan would be helpful.🧹 Remove or restore the commented test
If the test is obsolete, remove it:
- /* - public function testColKeys() - { - $table = new Table(null, array(), $this->debug); - // ... test body ... - } - */If the test needs updating, add a TODO or skip annotation:
/** * `@todo` Update for new Table abstraction */ public function testColKeys() { self::markTestSkipped('Needs update for new Table abstraction'); }
1182-1183: Debug line left in test provider.Line 1182 has a commented debug line that filters test cases. While harmless when commented, it could be removed for cleanliness.
- // return \array_intersect_key($tests, \array_flip(['traversableOfTraversable'])); return $tests;
| $cells = $row->getCells(); | ||
| $shortcode = $cells[0]->getValue(); | ||
| $links = $cells[2]->getValue(); | ||
| $cells[2]->setValue( | ||
| $links . ' <a href="#" data-toggle="#shortcode_' . $shortcode . '_doc" title="handler phpDoc"><i class="fa fa-code"></i></a>' | ||
| ); | ||
| $rowsNew[] = $this->buildInfoRow($row); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's read the file to see the actual code at the specified lines
cat -n src/Debug/Framework/WordPress/Shortcodes.php | sed -n '95,130p'Repository: bkdotcom/PHPDebugConsole
Length of output: 1569
🏁 Script executed:
#!/bin/bash
# Search for getValue() implementation to understand return type
rg -n 'getValue\(\)' src/Debug/Framework/WordPress/Shortcodes.php -B 2 -A 2Repository: bkdotcom/PHPDebugConsole
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Search for crateWithVals definition and usage
rg -n 'crateWithVals' --type=php | head -20Repository: bkdotcom/PHPDebugConsole
Length of output: 2484
🏁 Script executed:
#!/bin/bash
# Let's understand the structure - find where this Cell class is defined
rg -n 'class.*Cell' --type=php | head -10Repository: bkdotcom/PHPDebugConsole
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Find getValue() implementation in TableCell
rg -n 'public function getValue' --type=php -A 3 | grep -A 3 'TableCell\|Element'Repository: bkdotcom/PHPDebugConsole
Length of output: 349
🏁 Script executed:
#!/bin/bash
# Search for crateWithVals implementation
rg -n 'function crateWithVals' --type=php -A 5Repository: bkdotcom/PHPDebugConsole
Length of output: 604
🏁 Script executed:
#!/bin/bash
# Look for TableCell and Element class definitions
cat -n src/Table/Element.php | head -50Repository: bkdotcom/PHPDebugConsole
Length of output: 1480
🏁 Script executed:
#!/bin/bash
# Check TableCell class
cat -n src/Table/TableCell.php | head -50Repository: bkdotcom/PHPDebugConsole
Length of output: 1546
🏁 Script executed:
#!/bin/bash
# Check TableCell::setValue() implementation
cat -n src/Table/TableCell.php | sed -n '50,120p'Repository: bkdotcom/PHPDebugConsole
Length of output: 2136
🏁 Script executed:
#!/bin/bash
# Get more of crateWithVals implementation
cat -n src/Debug/Abstraction/Abstracter.php | sed -n '174,210p'Repository: bkdotcom/PHPDebugConsole
Length of output: 1763
🏁 Script executed:
#!/bin/bash
# Search for Abstraction class definition to understand structure
rg -n 'class Abstraction' --type=php | head -5Repository: bkdotcom/PHPDebugConsole
Length of output: 316
🏁 Script executed:
#!/bin/bash
# Check what the shortcode cells contain - look at where table is created
rg -n 'getCells\|TableCell' src/Debug/Framework/WordPress/Shortcodes.php -B 2 -A 2Repository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look at where the table is created and populated in Shortcodes.php
cat -n src/Debug/Framework/WordPress/Shortcodes.php | head -100Repository: bkdotcom/PHPDebugConsole
Length of output: 3252
🏁 Script executed:
#!/bin/bash
# Search for where this shortcodes method is called or how table is built
rg -n 'shortcodes|Shortcodes' src/Debug/Framework/WordPress/Shortcodes.php | head -20Repository: bkdotcom/PHPDebugConsole
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Look for the full class and method context
cat -n src/Debug/Framework/WordPress/Shortcodes.php | sed -n '80,110p'Repository: bkdotcom/PHPDebugConsole
Length of output: 1292
🏁 Script executed:
#!/bin/bash
# Check Abstraction class to understand the structure
cat -n src/Debug/Abstraction/Abstraction.php | sed -n '1,100p'Repository: bkdotcom/PHPDebugConsole
Length of output: 3118
🏁 Script executed:
#!/bin/bash
# Find getShortCodeData to see how cells are populated
cat -n src/Debug/Framework/WordPress/Shortcodes.php | sed -n '130,200p'Repository: bkdotcom/PHPDebugConsole
Length of output: 2940
🏁 Script executed:
#!/bin/bash
# Check if methodTable plugin creates cells with Abstraction values
rg -n 'methodTable|doTable' src/Debug/Framework/ --type=php -lRepository: bkdotcom/PHPDebugConsole
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Search for how table cells are created and what values are set
rg -n 'TableCell\|setValue' src/Debug/ --type=php -B 2 -A 2 | grep -A 5 'crateWithVals'Repository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the methodTable plugin to understand how it processes table data
find src/Debug -name "*methodTable*" -o -name "*MethodTable*" | head -5Repository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find the methodTable plugin
find src -name "*ethodTable*" -o -name "*method*table*" -type f | grep -i tableRepository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for getPlugin('methodTable')
rg -n "methodTable" src/Debug --type=php -B 2 -A 2Repository: bkdotcom/PHPDebugConsole
Length of output: 2421
🏁 Script executed:
#!/bin/bash
# Look for Route/Html/Table which might be the methodTable plugin
ls -la src/Debug/Dump/ | grep -i tableRepository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check Dump directory for Table-related files
find src/Debug -path "*Table*" -name "*.php" -type f | head -10Repository: bkdotcom/PHPDebugConsole
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Check the methodTable plugin (Method/Table.php)
cat -n src/Debug/Plugin/Method/Table.php | head -100Repository: bkdotcom/PHPDebugConsole
Length of output: 3223
🏁 Script executed:
#!/bin/bash
# Search for doTable method in Table.php
rg -n 'doTable' src/Debug/Plugin/Method/Table.php -A 30Repository: bkdotcom/PHPDebugConsole
Length of output: 3986
🏁 Script executed:
#!/bin/bash
# Look for where cells are populated with values
rg -n 'setValue|TableCell' src/Debug/Plugin/Method/Table.php -B 2 -A 2Repository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find valsFromLogEntry method
rg -n 'valsFromLogEntry' src/Debug/Plugin/Method/Table.php -A 30Repository: bkdotcom/PHPDebugConsole
Length of output: 2254
🏁 Script executed:
#!/bin/bash
# Check TableFactory to see how cells are created
rg -n 'new TableCell|setValue' src/Table/Factory.php -B 2 -A 2 | head -50Repository: bkdotcom/PHPDebugConsole
Length of output: 857
🏁 Script executed:
#!/bin/bash
# See what getShortCodeData returns
cat -n src/Debug/Framework/WordPress/Shortcodes.php | sed -n '195,230p'Repository: bkdotcom/PHPDebugConsole
Length of output: 1635
🏁 Script executed:
#!/bin/bash
# Check if values go through abstraction
rg -n 'abstracter|crateWithVals' src/Debug/Plugin/Method/Table.phpRepository: bkdotcom/PHPDebugConsole
Length of output: 139
🏁 Script executed:
#!/bin/bash
# See what type of values are in the shortcodes table
cat -n src/Debug/Framework/WordPress/Shortcodes.php | sed -n '195,225p'Repository: bkdotcom/PHPDebugConsole
Length of output: 1428
🏁 Script executed:
#!/bin/bash
# Test to understand Abstraction concatenation behavior
python3 << 'EOF'
# Check if Abstraction extends ArrayObject or array-like
import subprocess
result = subprocess.run(['rg', '-n', 'class Abstraction', 'src/Debug/Abstraction/Abstraction.php', '-A 5'],
capture_output=True, text=True)
print(result.stdout)
print("---")
result = subprocess.run(['rg', '-n', 'extends', 'src/Debug/Abstraction/Abstraction.php', '-A 2'],
capture_output=True, text=True)
print(result.stdout)
EOFRepository: bkdotcom/PHPDebugConsole
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Verify Abstraction class structure
cat -n src/Debug/Abstraction/Abstraction.php | sed -n '27,35p'Repository: bkdotcom/PHPDebugConsole
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Check how Abstraction is used as array-like (if at all)
rg -n 'Abstraction implements' src/Debug/Abstraction/Abstraction.php -A 2Repository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify the actual test to see if there's a case of Abstraction concatenation
rg -n 'to string' tests/ --type=php -i -B 2 -A 2 | head -30Repository: bkdotcom/PHPDebugConsole
Length of output: 470
🏁 Script executed:
#!/bin/bash
# Check how Abstraction's __toString is implemented in detail
cat -n src/Debug/Abstraction/Abstraction.php | sed -n '48,65p'Repository: bkdotcom/PHPDebugConsole
Length of output: 702
🏁 Script executed:
#!/bin/bash
# Verify what happens when Abstraction is concatenated
rg -n '__toString' src/Debug/Abstraction/ --type=php -B 2 -A 10Repository: bkdotcom/PHPDebugConsole
Length of output: 12954
🏁 Script executed:
#!/bin/bash
# Check tests to see if there's any test for Abstraction concatenation
rg -n 'Abstraction.*\.' tests/ --type=php | grep -i concat | head -5Repository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Let me trace through the actual execution - what exactly happens in modifyShortcodeTable
# The cells[2] contains value from getShortCodeData which wraps links with crateWithVals
# So getValue() returns Abstraction object
# Then concatenation calls __toString()
# Then setValue() stores the concatenated string (losing metadata)
# Check if there's any issue with this by looking at how the cell is rendered
cat -n src/Table/TableCell.php | sed -n '60,90p'Repository: bkdotcom/PHPDebugConsole
Length of output: 1097
Preserve Abstraction metadata when concatenating values and storing back to cells.
At line 101/121: $links and $method are Abstraction objects returned from getValue(). Direct string concatenation (line 103, 122) calls __toString(), extracting only the raw value and discarding metadata like addQuotes and sanitize flags. When the concatenated string is stored back via setValue(), the metadata is permanently lost, causing the HTML <a> tag and its attributes to be unexpectedly escaped or sanitized.
Extract the string value from Abstraction objects, perform concatenation, then re-wrap the result with crateWithVals() to restore metadata:
Suggested fix
- $links = $cells[2]->getValue();
- $cells[2]->setValue(
- $links . ' <a href="#" data-toggle="#shortcode_' . $shortcode . '_doc" title="handler phpDoc"><i class="fa fa-code"></i></a>'
- );
+ $linksVal = $cells[2]->getValue();
+ if ($linksVal instanceof Abstraction && isset($linksVal['value'])) {
+ $linksVal = $linksVal['value'];
+ }
+ $linksHtml = $linksVal . ' <a href="#" data-toggle="#shortcode_' . $shortcode . '_doc" title="handler phpDoc"><i class="fa fa-code"></i></a>';
+ $cells[2]->setValue($this->debug->abstracter->crateWithVals(
+ $linksHtml,
+ array(
+ 'addQuotes' => false,
+ 'sanitize' => false,
+ )
+ ));
@@
- $method = $cells[1]->getValue();
- $phpDoc = $this->debug->phpDoc->getParsed($method . '()');
+ $methodVal = $cells[1]->getValue();
+ if ($methodVal instanceof Abstraction && isset($methodVal['value'])) {
+ $methodVal = $methodVal['value'];
+ }
+ $phpDoc = $this->debug->phpDoc->getParsed($methodVal . '()');Also applies to: 119-123
🤖 Prompt for AI Agents
In `@src/Debug/Framework/WordPress/Shortcodes.php` around lines 99 - 105, The code
is concatenating Abstraction objects returned by $cells[2]->getValue() (e.g.,
$links and similarly $method) which calls __toString() and drops metadata;
instead, extract the raw string/value from the Abstraction, perform the
concatenation to build the HTML fragment, then re-wrap the resulting string with
crateWithVals() (preserving flags like addQuotes/sanitize) before calling
$cells[2]->setValue(...); apply the same pattern for the other occurrence around
$method so metadata is retained when calling setValue() and buildInfoRow().
| public function unserialize($data) | ||
| { | ||
| /** @var mixed */ | ||
| $unserialized = \unserialize($data); | ||
| if (\is_array($unserialized)) { | ||
| $this->__unserialize($unserialized); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant lines
fd -t f "Element.php" | grep -E "src/Table"Repository: bkdotcom/PHPDebugConsole
Length of output: 89
🏁 Script executed:
# Get the exact content around lines 466-471
if [ -f "src/Table/Element.php" ]; then
sed -n '460,475p' src/Table/Element.php
fiRepository: bkdotcom/PHPDebugConsole
Length of output: 415
🏁 Script executed:
# Search for all unserialize() calls in the codebase to understand the scope
rg "unserialize\(" --type phpRepository: bkdotcom/PHPDebugConsole
Length of output: 3479
🌐 Web query:
PHP unserialize allowed_classes parameter security best practices
💡 Result:
Best practices for unserialize() + allowed_classes (security)
-
Prefer not to use
unserialize()at all for untrusted data
Use a data-only format like JSON instead. PHP explicitly warns that untrusted input must not be passed tounserialize()even ifallowed_classesis set, because object instantiation/autoloading can still lead to exploitation. [1][2] -
If you must use
unserialize(), default to “no objects”$value = unserialize($data, ['allowed_classes' => false]);
This prevents object creation (disallowed classes become
__PHP_Incomplete_Class). [1] -
If objects are required, use a strict allow-list (not
true, not “option omitted”)
Omittingallowed_classesis equivalent to allowing all classes. Use an explicit allow-list of the minimum safe classes and audit them carefully. [1][4] -
Assume deserialization = potential “object injection” surface
Attacks typically rely on existing classes with dangerous magic methods like__wakeup()/__destruct()and “POP chains”. Treat any deserialized values as attacker-controlled and design classes accordingly. [2] -
Add integrity protection for externally stored serialized blobs
If the serialized payload is stored client-side or otherwise exposed, sign/verify it (e.g., HMAC) so tampering is detected before deserialization (PHP manual specifically suggests this). [1] -
Constrain resource/DoS risk
Usemax_depthto limit nesting and reduce risk of crashes/stack overflows:unserialize($data, ['allowed_classes' => false, 'max_depth' => 64]);
[1]
-
Operational guardrails
- Avoid/limit autoload side effects around deserialization (autoloading can be part of the exploit path). [1][2]
- Add SAST/lint rules to flag
unserialize()calls missing restrictiveallowed_classes. [4]
Key takeaway: allowed_classes is risk-reduction, not a guarantee—don’t unserialize untrusted input; otherwise block objects (false) or tightly allow-list, and protect integrity + depth. [1][2]
🏁 Script executed:
# Check minimum PHP version requirement
if [ -f "composer.json" ]; then
cat composer.json | jq '.require.php' 2>/dev/null || grep -A2 '"php"' composer.json
fiRepository: bkdotcom/PHPDebugConsole
Length of output: 90
🏁 Script executed:
# Check the Php.php file to see how it handles allowed_classes for compatibility
cat -n src/Debug/Utility/Php.php | grep -A 10 -B 5 "allowed_classes"Repository: bkdotcom/PHPDebugConsole
Length of output: 859
Harden unserialize() against object injection.
Unrestricted unserialize() can instantiate arbitrary classes from untrusted input. Restrict allowed_classes to prevent object injection. Since the code only expects arrays (as evidenced by the if (\is_array($unserialized)) check), block all object instantiation.
🛡️ Suggested fix
- $unserialized = \unserialize($data);
+ $unserialized = \unserialize($data, ['allowed_classes' => false]);Note: The allowed_classes parameter is available in PHP 7.0+. For PHP 5.4 compatibility, wrap the call in a version check (e.g., PHP_VERSION_ID >= 70000) as shown in src/Debug/Utility/Php.php.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function unserialize($data) | |
| { | |
| /** @var mixed */ | |
| $unserialized = \unserialize($data); | |
| if (\is_array($unserialized)) { | |
| $this->__unserialize($unserialized); | |
| public function unserialize($data) | |
| { | |
| /** `@var` mixed */ | |
| $unserialized = \unserialize($data, ['allowed_classes' => false]); | |
| if (\is_array($unserialized)) { | |
| $this->__unserialize($unserialized); |
🤖 Prompt for AI Agents
In `@src/Table/Element.php` around lines 466 - 471, The unserialize method
currently calls \unserialize($data) which allows object instantiation from
untrusted input; change it to use the allowed_classes option to block object
injection (e.g. call \unserialize($data, ['allowed_classes' => false])) and keep
the existing check that only arrays are passed to __unserialize; for PHP
versions older than 7 (PHP_VERSION_ID < 70000) fall back to the current
\unserialize($data) behavior behind a version check so compatibility with PHP
5.x is preserved, referencing the unserialize method and the __unserialize call
to locate the change.
| public function setChildren(array $children) | ||
| { | ||
| $children = \array_map(static function ($child) { | ||
| return $child instanceof Element | ||
| ? $child | ||
| : new TableCell($child); | ||
| }, $children); | ||
| return parent::setChildren($children); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and find TableRow.php
find . -name "TableRow.php" -type fRepository: bkdotcom/PHPDebugConsole
Length of output: 126
🏁 Script executed:
# Let's look at the TableRow class to understand its parent and the setChildren method
cat -n src/Table/TableRow.php | head -100Repository: bkdotcom/PHPDebugConsole
Length of output: 2384
🏁 Script executed:
# Find the Element class and its appendChild method
ast-grep --pattern $'class Element {
$$$
public function appendChild($$$) {
$$$
}
$$$
}'Repository: bkdotcom/PHPDebugConsole
Length of output: 50
🏁 Script executed:
# Also search for any type hints or documentation about what can be added as children
rg "TableRow|TableCell" -A 3 -B 3 src/Table/ | head -80Repository: bkdotcom/PHPDebugConsole
Length of output: 3362
🏁 Script executed:
# Find the Element class
find . -name "Element.php" -type fRepository: bkdotcom/PHPDebugConsole
Length of output: 91
🏁 Script executed:
# Look at the Element class and find appendChild and setChildren methods
cat -n src/Table/Element.php | head -150Repository: bkdotcom/PHPDebugConsole
Length of output: 4865
🏁 Script executed:
# Search for appendChild implementation in Element
rg "function appendChild" src/Table/ -A 10Repository: bkdotcom/PHPDebugConsole
Length of output: 611
🏁 Script executed:
# Search for setChildren implementation in Element
rg "function setChildren" src/Table/ -A 10Repository: bkdotcom/PHPDebugConsole
Length of output: 1662
Use TableCell instead of Element in the instanceof check.
The setChildren method checks $child instanceof Element rather than $child instanceof TableCell. This is inconsistent with the class contract: getCells() returns array<TableCell>, setCells() expects array<TableCell>|array<array>, and appendCell() enforces TableCell explicitly. Allowing any Element subclass undermines type safety and could result in non-TableCell elements being stored in the row.
🤖 Prompt for AI Agents
In `@src/Table/TableRow.php` around lines 76 - 84, The setChildren method
currently treats any Element as acceptable; change the instanceof check to
require TableCell so only TableCell instances are preserved: in
TableRow::setChildren, map $children and replace the conditional ($child
instanceof Element) with ($child instanceof TableCell), otherwise construct new
TableCell($child), then call parent::setChildren($children) as before to ensure
rows only contain TableCell objects.
| self::$tempData = array( | ||
| 'columnsMeta' => $table->getMeta('columns'), | ||
| 'headerVals' => \array_map(static function ($cell) { | ||
| return $cell->getValue(); | ||
| }, $table->getHeader()->getChildren()), | ||
| 'rowKey' => 0, | ||
| ); |
There was a problem hiding this comment.
Missing null check on getHeader() before calling getChildren().
If the table has no header ($table->getHeader() returns null), line 37-38 will cause a fatal error when calling ->getChildren() on null.
🐛 Proposed fix to add null check
self::$tempData = array(
'columnsMeta' => $table->getMeta('columns'),
- 'headerVals' => \array_map(static function ($cell) {
- return $cell->getValue();
- }, $table->getHeader()->getChildren()),
+ 'headerVals' => $table->getHeader()
+ ? \array_map(static function ($cell) {
+ return $cell->getValue();
+ }, $table->getHeader()->getChildren())
+ : array(),
'rowKey' => 0,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self::$tempData = array( | |
| 'columnsMeta' => $table->getMeta('columns'), | |
| 'headerVals' => \array_map(static function ($cell) { | |
| return $cell->getValue(); | |
| }, $table->getHeader()->getChildren()), | |
| 'rowKey' => 0, | |
| ); | |
| self::$tempData = array( | |
| 'columnsMeta' => $table->getMeta('columns'), | |
| 'headerVals' => $table->getHeader() | |
| ? \array_map(static function ($cell) { | |
| return $cell->getValue(); | |
| }, $table->getHeader()->getChildren()) | |
| : array(), | |
| 'rowKey' => 0, | |
| ); |
🤖 Prompt for AI Agents
In `@src/Table/Utility.php` around lines 34 - 40, self::$tempData assignment calls
$table->getHeader()->getChildren() without checking for null, causing a fatal
error when getHeader() returns null; update the 'headerVals' computation in the
self::$tempData block to first check $table->getHeader() (or retrieve
$table->getHeader() into a variable) and only call ->getChildren() when it's not
null, otherwise use an empty array for headerVals, leaving the array_map logic
(and the 'columnsMeta' and 'rowKey' entries) unchanged so behavior is preserved
when a header exists.
There was a problem hiding this comment.
Pull request overview
Introduces a new internal bdk\Table module to represent tabular data as structured objects and refactors Debug’s table/trace/profile rendering and serialization to use this structure across HTML/text/script/FirePHP/WAMP outputs.
Changes:
- Added
bdk\Table\Table,TableRow,TableCell,Factory, andUtilityto standardize table creation/serialization. - Refactored Debug’s table/trace/profile pipelines and multiple routes/dumpers to emit/consume table abstractions consistently.
- Tightened many PHP method signatures to use
arraytype hints and updated tests/fixtures accordingly.
Reviewed changes
Copilot reviewed 149 out of 150 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/docroot/frontController.php | Switches globals request creation to ServerRequestUtility::fromGlobals(). |
| tests/Table/Fixture/Stringable.php | Adds a __toString() fixture for table-related tests. |
| tests/PubSub/Fixture/ValueStore.php | Adds array typehint to onSet in test fixture. |
| tests/Debug/data/PropertyHooks.php | Updates expected abstraction fixture (traverseValues no longer present). |
| tests/Debug/data/PropertyAsymVisibility.php | Updates expected abstraction fixture (traverseValues no longer present). |
| tests/Debug/data/ConfusableIdentifiers.php | Updates expected abstraction fixture (removes traverseValues). |
| tests/Debug/Utility/SerializeLogTest.php | Updates serialization/back-compat test expectations (but introduces a test discovery issue). |
| tests/Debug/Utility/PhpTypeTest.php | Renames enum test case key (enum → enum.pure). |
| tests/Debug/Utility/PhpTest.php | Renames enum test case key (enum → enum.pure). |
| tests/Debug/Type/StringTest.php | Updates expected table HTML/structure and entry shape for script output. |
| tests/Debug/Type/EnumTest.php | Updates expected table HTML structure for enums. |
| tests/Debug/ServiceProviderTest.php | Updates where Html\Table is accessed (now under valDumper). |
| tests/Debug/Route/WampTest.php | Updates expected trace payload shape to new table abstraction format. |
| tests/Debug/Route/Html/ErrorSummaryTest.php | Updates expected trace table markup formatting. |
| tests/Debug/Route/ChromeLoggerTest.php | Uses getRoute('chromeLogger') accessor instead of direct property. |
| tests/Debug/Plugin/PrettifyTest.php | Adjusts expected XML string formatting. |
| tests/Debug/Plugin/Method/ProfileTest.php | Updates profile table assertions to new table abstraction shape. |
| tests/Debug/Plugin/Method/GeneralTest.php | Updates getDump signature usage to include route discriminator. |
| tests/Debug/Plugin/Method/BasicTest.php | Updates trace/table expectations to new abstraction usage. |
| tests/Debug/Plugin/LogResponseTest.php | Updates headers table assertions to new table structure. |
| tests/Debug/Plugin/LogRequestTest.php | Updates request/cookie table expectations to new table structure and meta. |
| tests/Debug/LoggerTest.php | Updates logged table expectations to full table abstraction array form. |
| tests/Debug/Framework/WordPress/ShortcodesTest.php | Updates shortcode table expectations to new table structure + child/info row layout. |
| tests/Debug/Framework/WordPress/ObjectCacheTest.php | Updates cache table expectations to new table structure (header/footer/rows). |
| tests/Debug/Framework/WordPress/HooksTest.php | Updates hooks table expectations to new table structure (header/footer/rows). |
| tests/Debug/DebugTestFramework.php | Tweaks setup behavior, test harness defaults, and abstraction key assertions. |
| tests/Debug/Collector/StatementInfoTest.php | Updates expected JSON log format for parameter tables. |
| tests/Debug/Collector/PdoTest.php | Updates expected parameter table structure. |
| tests/Debug/Collector/MysqliTest.php | Updates expected parameter table structure in serialized log output. |
| tests/Debug/Collector/MonologHandlerTest.php | Updates table expectations to new table abstraction structure. |
| tests/Debug/Collector/DoctrineMiddlewareTest.php | Updates expected HTML table output and enum/PDO-type handling strings. |
| tests/Debug/Collector/DoctrineLoggerTest.php | Updates expected HTML table output formatting. |
| src/Teams/Elements/Table.php | Tightens constructor typing for Teams table element rows. |
| src/Table/Utility.php | Adds conversion of bdk\Table\Table to array for dumpers/routes. |
| src/Table/TableRow.php | Adds TableRow element with cell normalization. |
| src/Table/TableCell.php | Adds TableCell element with rendering hooks and default value dumping. |
| src/Table/Table.php | Adds Table element with caption/header/footer/body structure and serialization. |
| src/Slack/BlockFactory.php | Adds array typehints for $values params in multiple builders. |
| src/Slack/BlockElementsFactory.php | Adds array typehints for $values params in multiple element builders. |
| src/PubSub/ValueStore.php | Adds array typehint to onSet. |
| src/Promise/Create.php | Import ordering tweak. |
| src/Promise/AbstractResolvedPromise.php | Updates constructor docblock param descriptions. |
| src/I18n/I18n.php | Adds array typehint for $cfg. |
| src/ErrorHandler/Plugin/StatsStoreFile.php | Adds array typehints for ctor and postSetCfg. |
| src/ErrorHandler/Plugin/Stats.php | Adds array typehints for ctor and postSetCfg. |
| src/ErrorHandler/Plugin/Emailer.php | Adds array typehint for $cfg. |
| src/ErrorHandler/ErrorHandler.php | Adds array typehints across ctor, getInstance, and handler methods. |
| src/ErrorHandler/AbstractErrorHandler.php | Adds array typehint to postSetCfg. |
| src/ErrorHandler/AbstractError.php | Adds array typehint to onSet. |
| src/ErrorHandler/AbstractComponent.php | Adds array typehint to postSetCfg. |
| src/Debug/js_src/enhanceEntries.js | Minor JS formatting change (removes semicolon). |
| src/Debug/js_src/Tooltip.js | Simplifies jQuery tag creation (<dt>/<dd>). |
| src/Debug/js_src/FileLinks.js | Adjusts trace link creation to skip context rows and fix colspan. |
| src/Debug/js/Debug.js | Built artifact updates matching JS source changes. |
| src/Debug/Utility/Utility.php | Updates doc comment wording for translation wrapper args. |
| src/Debug/Utility/StopWatch.php | Adds array typehint for ctor $vals. |
| src/Debug/Utility/Profile.php | Adds array typehint for ctor $namespacesIgnore. |
| src/Debug/Utility/PhpType.php | Improves closure detection and doc wording; imports Closure. |
| src/Debug/Utility/PhpDoc.php | Adds array typehint for ctor $cfg. |
| src/Debug/Utility/HtmlBuild.php | Adds array typehint for internal attrib builder helper. |
| src/Debug/Utility/FileTree.php | Adds array typehints for filesToTree args. |
| src/Debug/Utility/FileStreamWrapper.php | Import ordering tweak. |
| src/Debug/Route/WampCrate.php | Adds table-specific crating and ensures trace meta is crated. |
| src/Debug/Route/Text.php | Ensures text dumper is always initialized. |
| src/Debug/Route/Stream.php | Adds array typehint to postSetCfg. |
| src/Debug/Route/Script.php | Uses route-specific base dumper config and simplifies string replacement logic. |
| src/Debug/Route/Html/FatalError.php | Ensures trace log entry is crated before dumping. |
| src/Debug/Route/Html.php | Adds array typehint to postSetCfg. |
| src/Debug/Route/Firephp.php | Uses route-specific base dumper config and converts table abstraction to FirePHP table format. |
| src/Debug/Route/ChromeLogger.php | Uses route-specific base dumper config; adds array typehint to reduce fill. |
| src/Debug/Route/AbstractRoute.php | Adds array typehint to channel-key matcher arg. |
| src/Debug/Psr15/Middleware.php | Adds array typehint for $cfg. |
| src/Debug/Plugin/Prettify.php | Trims trailing whitespace from pretty-printed XML output. |
| src/Debug/Plugin/Method/Trace.php | Refactors trace generation to use table plugin and table structure/meta updates. |
| src/Debug/Plugin/Method/Table.php | Refactors table method to use bdk\Table\Factory + structured meta handling. |
| src/Debug/Plugin/Method/Profile.php | Ensures profile output uses table plugin and updates key column values to identifier abstractions. |
| src/Debug/Plugin/Method/GroupCleanup.php | Removes redundant stack count bookkeeping. |
| src/Debug/Plugin/Method/Group.php | Adds array typehint to autoArgs. |
| src/Debug/Plugin/Method/General.php | Extends dumper retrieval to support route-specific instances; container integration tweaks. |
| src/Debug/Plugin/LogRequest.php | Passes detectFiles meta when logging $_FILES. |
| src/Debug/LogEntry.php | Tightens constructor arg typing; fixes detectFiles cfg propagation; adds array typehint to onSet. |
| src/Debug/Framework/Yii2/LogTarget.php | Adds array typehint for $config. |
| src/Debug/Framework/Yii1_1/LogRoute.php | Adds array typehint for $opts. |
| src/Debug/Framework/WordPress/WpHttp.php | Adds array typehint to postSetCfg. |
| src/Debug/Framework/WordPress/WpDb.php | Adds array typehint to postSetCfg. |
| src/Debug/Framework/WordPress/WordPress.php | Adds array typehint to postSetCfg. |
| src/Debug/Framework/WordPress/Shortcodes.php | Reworks shortcode output to build/modify a structured table and inserts phpDoc rows via TableRow/TableCell. |
| src/Debug/Framework/WordPress/ObjectCache.php | Migrates table meta config to new column meta format. |
| src/Debug/Framework/WordPress/Hooks.php | Migrates hooks table meta to new column labels/meta/total cols format. |
| src/Debug/Framework/Laravel/ServiceProvider.php | Refactors model usage table building to structured rows and new table meta format. |
| src/Debug/Framework/Laravel/LogViews.php | Simplifies view logging call (relies on explicit data-file attrib). |
| src/Debug/Framework/Laravel/EventsSubscriber.php | Adds array typehint for $payload. |
| src/Debug/Framework/Laravel/CacheEventsSubscriber.php | Adds array typehint for $options. |
| src/Debug/Dump/TextAnsi/Value.php | Adds table dumping; refactors identifier markup/highlighting and numeric/date formatting. |
| src/Debug/Dump/TextAnsi/TextAnsiObject.php | Uses identifier type constants and adds underlined methods header styling. |
| src/Debug/Dump/TextAnsi.php | Refactors val dumper initialization and renames to initValDumper. |
| src/Debug/Dump/Text/Value.php | Adds table dumping for plain text output. |
| src/Debug/Dump/Text/TextObject.php | Uses identifier type constants for markup. |
| src/Debug/Dump/Text.php | Refactors val dumper init; routes tabular output through default handling. |
| src/Debug/Dump/Substitution.php | Adds array typehints for args/options. |
| src/Debug/Dump/Html/Value.php | Adds table dumping support; improves timestamp container wrapping; supports trueAs/falseAs. |
| src/Debug/Dump/Html/Object/Enum.php | Prevents type-class output for brief enum rendering via dumpType=false. |
| src/Debug/Dump/Html/Object/AbstractSection.php | Uses identifier type constants in markup. |
| src/Debug/Dump/Html/HtmlStringBinary.php | Refactors post-dump builder and avoids leaking attribs in wrapped output. |
| src/Debug/Dump/Html/HtmlString.php | Refactors binary substitution handling and prettified wrapping logic. |
| src/Debug/Dump/Html/HtmlObject.php | Uses identifier type constants for attribute markup. |
| src/Debug/Dump/Html/Helper.php | Removes trace-context row injection logic from helper (handled elsewhere). |
| src/Debug/Dump/Html/Group.php | Uses identifier type constants for function-name markup. |
| src/Debug/Dump/Html.php | Removes dedicated Html\Table usage in favor of Value-driven dumping and default formatting. |
| src/Debug/Dump/Base/Value.php | Adds table dumping for base dumper paths (ChromeLogger/FirePHP/Script/etc). |
| src/Debug/Dump/Base.php | Adds route-specific undefined handling config and refactors tabular processing. |
| src/Debug/Dump/AbstractValue.php | Adds config storage, tracks previous options, and formalizes abstract dump hooks incl. table dumping. |
| src/Debug/Debug.php | Adds array typehint for ctor and getInstance. |
| src/Debug/Config.php | Tightens typing for internal config path args. |
| src/Debug/Collector/MonologHandler/CompatTrait_3.0.php | Adds PHPCS disable for duplicate class name within compat trait guard. |
| src/Debug/Collector/GuzzleMiddleware.php | Adds array typehint for $cfg. |
| src/Debug/Collector/CurlHttpMessageMiddleware.php | Adds array typehint for $cfg. |
| src/Debug/Collector/AbstractAsyncMiddleware.php | Adds array typehint for $cfg. |
| src/Debug/Abstraction/Type.php | Adds table type constant and better object subtype detection (closure/enum). |
| src/Debug/Abstraction/Object/Subscriber.php | Refactors object abstraction lifecycle hooks and adds special handling for bdk\Table objects. |
| src/Debug/Abstraction/Object/PropertiesInstance.php | Removes isTraverseOnly guard (traverseValues path removed). |
| src/Debug/Abstraction/Object/MethodParams.php | Adds array typehint for buildValues. |
| src/Debug/Abstraction/Object/Helper.php | Removes isTraverseOnly helper (no longer used). |
| src/Debug/Abstraction/Object/Abstraction.php | Adds unstructuredValue handling and tightens ctor typing. |
| src/Debug/Abstraction/Object/AbstractInheritable.php | Adds array typehint for buildValues. |
| src/Debug/Abstraction/Abstraction.php | Adds array typehint for ctor values and onSet. |
| src/Debug/Abstraction/Abstracter.php | Adds detectFiles cfg; tightens typing for crate APIs; adjusts abstraction detection logic. |
| src/Debug/Abstraction/AbstractString.php | Renames $maxlen to $maxLen for consistency. |
| src/Debug/Abstraction/AbstractObject.php | Returns unstructured abstractions for elements/tables; adjusts root instance subscription behavior. |
| src/Debug/AbstractDebug.php | Adds array typehint for ctor; changes fallback custom method handling to call root log(). |
| src/CurlHttpMessage/Handler/CurlMulti.php | Adds array typehint for ctor $options. |
| src/CurlHttpMessage/Factory.php | Adds array typehints for ctor/options/headers in builder methods. |
| src/CurlHttpMessage/AbstractClient.php | Adds array typehint for ctor $options. |
| src/Container/ObjectBuilder.php | Refactors build semantics and docs; adds useContainer flag. |
| src/Container/Container.php | Adds getObject($useContainer) and tightens typing for ctor/setValues. |
| src/Backtrace/Backtrace.php | Increases debug_backtrace depth limit. |
| phpunit.xml.dist | Adds a dedicated Table testsuite. |
| src/Debug/Utility/TableRow.php | Removes legacy table row utility (replaced by new table module). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…w, & TableCell) LogPhp::logPhpIni... if multiple ini files present, they weren't detected as file / linked
Update UnserializeLogTest
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 173 out of 174 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
New Table utilities / classes for representing table data
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests