![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
#60758 closed defect (bug) (fixed)
Interactivity API: Bind directives may access uninitialized string offset
| Reported by: | jonsurrell | Owned by: | swissspidy |
|---|---|---|---|
| Priority: | normal | Milestone: | 6.5 |
| Component: | Interactivity API | Version: | 6.5 |
| Severity: | normal | Keywords: | has-patch has-unit-tests commit fixed-major dev-reviewed |
| Cc: | Focuses: |
Description
When using bind directives with short attribute names (less than 5 characters), the following warning is produced:
Warning: Uninitialized string offset 4 in /var/www/html/wp-includes/interactivity-api/class-wp-interactivity-api.php on line 579
This should be fixed to prevent the warning.
Change History (21)
This ticket was mentioned in PR #6257 on WordPress/wordpress-develop by @jonsurrell.
2 years ago
#1
- Keywords has-patch added
@jonsurrell commented on PR #6257:
2 years ago
#2
👋 @c4rl0sbr4v0 @DAreRodz for review.
@swissspidy commented on PR #6257:
2 years ago
#4
Possible to add tests for this?
@jonsurrell commented on PR #6257:
2 years ago
#6
I tried adding tests and would like to, but I couldn't get them to fail on the warning, that may be a configuration option or my lack of knowledge, but I couldn't find any way of saying "warnings on _this test_ should cause the test to fail."
There is actually a test that has a short attribute and should hit this code path, but it doesn't fail the test suite or seem to generate any warning:
2 years ago
#7
I can see the error when running tests. I might be valuable to add another test that handles the boolean value:
The same check needs to be added in another line which only will trigger when passing true:
if ( is_bool( $result ) && strlen( $bound_attribute ) > 5 && '-' === $bound_attribute[4] ) {
@jonsurrell commented on PR #6257:
2 years ago
#8
Good catch! I pushed a change to apply the same fix to that warning.
2 years ago
#9
You also need a test like the one I shared to trigger the warning in the second updated line. The value needs to be true.
@jonsurrell commented on PR #6257:
2 years ago
#10
You also need a test like the one I shared to trigger the warning in the second updated line. The value needs to be
true.
I've pushed a test for this, but I don't see the warnings when running the tests 😕
@jonsurrell commented on PR #6257:
2 years ago
#11
You also need a test like the one I shared to trigger the warning in the second updated line. The value needs to be
true.
I've pushed a test for this, but I don't see the warnings when running the tests 😕
@jonsurrell commented on PR #6257:
2 years ago
#12
I pushed a test that does error if I revert the changes.
@jonsurrell commented on PR #6257:
2 years ago
#13
I'll address the test failures.
@jonsurrell commented on PR #6257:
2 years ago
#14
I fixed the tests and updated them to use assertSame. We're comparing values of types we expect to match, mostly strings, so assertSame is the appropriate assertion.
@swissspidy commented on PR #6257:
2 years ago
#17
Committed in https://core.trac.wordpress.org/changeset/57835
Fix the following warning when using an interactivity bind directive with a short attribute name, e.g.
wp-data-bind--id.Trac ticket: https://core.trac.wordpress.org/ticket/60758