Repository navigation
fix(kernel): reload XOBJ_DTYPE_UNICODE_ARRAY values intact - #271
Conversation
Two defects remained after XOOPS#269 made the array_walk() encode and decode calls act on the array: XoopsObject::assignVar() url-decoded the stored value as a whole string before getVar() unserialized it. Any element containing a %-sequence (café, a+b, 100%) changed length, unserialize() warned and getVar() returned []. The serialized string is now stored as is; getVar() unserializes it and decodes the elements. XoopsModelWrite::cleanVars() still ran the addslashes() loop that predates $db->quote() before url-encoding the elements, so the backslashes became stored data (O'Reilly became O\'Reilly). The loop is removed; the whole value is SQL-quoted once. A new test stores an array through both write paths in a subprocess with the real functions.encoding.php, reloads it through assignVar() and asserts the original array. It fails on master and passes here.
Reviewer's GuideFixes unicode-array round trips by preserving serialized payloads until unserialization, removing pre-encoding addslashes() corruption, and adding an end-to-end regression test for both write paths and problematic string values. Sequence diagram for Unicode array round tripsequenceDiagram
participant Writer as XoopsModelWrite
participant DB as Database
participant Object as XoopsObject
participant Value as getVar()
Writer->>Writer: array_walk(cleanv, xoops_aw_encode)
Writer->>DB: quote(serialize(cleanv))
DB-->>Object: stored serialized payload
Object->>Object: assignVar(key, value)
Object->>Value: getVar()
Value->>Value: unserialize(value)
Value->>Value: decode array elements
Value-->>Object: original Unicode array
Flow diagram for safe Unicode array encodingflowchart LR
A[Unicode array values] --> B[array_walk with xoops_aw_encode]
B --> C[serialize encoded array]
C --> D[Database quote once]
D --> E[Stored serialized payload]
E --> F[assignVar preserves payload]
F --> G[getVar unserializes and decodes elements]
G --> H[Original array values]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughUnicode-array handling changes in ChangesUnicode array persistence
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preserving XOBJ_DTYPE_UNICODE_ARRAY values during reload. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 PHPStan (2.2.16)
-
PHP Fatal error: Uncaught Error: Class "Nette\DI\ServiceCreationException" not found in /.cache/phpstan/phpstan/cache/nette.configurator/Container_3bdd9a7f53.php:9316
Stack trace:
#0phar:///usr/bin/phpstan/vendor/nette/di/src/DI/Container.php(51): Container_3bdd9a7f53->getDynamicParameter()
#1phar:///usr/bin/phpstan/vendor/nette/di/src/DI/Container.php(245): _PHPStan_08e3736ef\Nette\DI\Container->_PHPStan_08e3736ef\Nette\DI{closure}()
#2phar:///usr/bin/phpstan/vendor/nette/di/src/DI/Container.php(50): _PHPStan_08e3736ef\Nette\DI\Container->preventDeadLock()
#3[internal function]: _PHPStan_08e3736ef\Nette\DI\Container->getParameter()
#4/.cache/phpstan/phpstan/cache/nette.configurator/Container_3bdd9a7f53.php(9329): array_map()
#5phar:///usr/bin/phpstan/src/DependencyInjection/Nette/NetteContainer.php(94): Container_3bdd9a7f53->getParameters()
#6phar:///usr/bin/phpstan/src/DependencyInjection/MemoizingContainer.php(57): PHPStan\DependencyInjection\Nette\NetteContainer->getParame... [truncated 2699 characters] ...
an\Command\AnalyseCommand->execute()
#11phar:///usr/bin/phpstan/vendor/symfony/console/Application.php(868): _PHPStan_08e3736ef\Symfony\Component\Console\Command\Command->run()
#12phar:///usr/bin/phpstan/vendor/symfony/console/Application.php(261): _PHPStan_08e3736ef\Symfony\Component\Console\Application->doRunCommand()
#13phar:///usr/bin/phpstan/vendor/symfony/console/Application.php(157): _PHPStan_08e3736ef\Symfony\Component\Console\Application->doRun()
#14phar:///usr/bin/phpstan/bin/phpstan(116): _PHPStan_08e3736ef\Symfony\Component\Console\Application->run()
#15phar:///usr/bin/phpstan/bin/phpstan(117): _PHPStan_08e3736ef{closure}()
#16/usr/bin/phpstan(7): require('...')
#17{main}
thrown in /.cache/phpstan/phpstan/cache/nette.configurator/Container_3bdd9a7f53.php on line 9316
Comment @coderabbitai help to get the list of available commands.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #271 +/- ##
=========================================
Coverage 22.54% 22.54%
+ Complexity 8672 8671 -1
=========================================
Files 685 685
Lines 46330 46328 -2
=========================================
Hits 10444 10444
+ Misses 35886 35884 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="htdocs/kernel/object.php" line_range="268" />
<code_context>
$value = $temp;
$this->vars[$key]['value'] = $value;
} else {
- $this->vars[$key]['value'] = xoops_convert_decode($value);
</code_context>
<issue_to_address>
**Legacy apostrophes retain backslashes**
When a Unicode-array row written by the previous model path contains an apostrophe, `assignVar()` preserves the legacy serialized payload, and `getVar()` decodes its elements without removing the old writer’s extra backslash. The value reloads as `O\'Reilly` instead of `O'Reilly`, so upgraded installations display it incorrectly and saving it again preserves the slash.
Normalize legacy array elements to remove the old writer’s extra escape before returning them from `getVar()`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the encoding or reload logic is still wrong, saving a Unicode-array field can persist malformed serialized data, and reverting the code will not restore rows already written; those records would need repair.
Blocking findings: htdocs/kernel/object.php:268
Summary
Two defects remained in the
XOBJ_DTYPE_UNICODE_ARRAYround trip after #269 made thearray_walk()encode and decode calls act on the array:XoopsObject::assignVar()url-decoded the stored value as a whole string beforegetVar()unserialized it. Any element containing a%-sequence (café,a+b,100%) changed length,unserialize()warned andgetVar()returned[]. The serialized string is now stored as is;getVar()unserializes it and decodes the elements.XoopsModelWrite::cleanVars()still ran theaddslashes()loop that predates$db->quote()before url-encoding the elements, so the backslashes became stored data (O'ReillybecameO\'Reilly). The loop is removed; the whole value is SQL-quoted once.Test
tests/unit/htdocs/kernel/XoopsObjectUnicodeArrayTest.phpstores an array through both write paths in a subprocess with the realfunctions.encoding.php, reloads it throughassignVar()and asserts the original array.O%5C%27Reillystored)phpunit --filter 'Object|Write|Encoding'Changelog line added under the 2.7.4 NOT RELEASED section.
Summary by Sourcery
Fix unicode array serialization and reload handling so stored values round-trip without corruption.
Bug Fixes:
Documentation:
Tests:
Summary by CodeRabbit