Skip to content

fix: esc() for 'raw' context - #8633

Merged
kenjis merged 1 commit into
codeigniter4:developfrom
Cleric-K:develop
Mar 22, 2024
Merged

kenjis merged 1 commit into
codeigniter4:developfrom
Cleric-K:develop

Conversation

@Cleric-K

@Cleric-K Cleric-K commented Mar 18, 2024

Copy link
Copy Markdown
Contributor

Description
Fixes #8624

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis changed the title esc() for 'raw' context (Fixes #8624) fix: esc() for 'raw' context Mar 18, 2024
@kenjis

kenjis commented Mar 18, 2024

Copy link
Copy Markdown
Member

@Cleric-K

Copy link
Copy Markdown
Contributor Author

Please sign all commits. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.md

Sorry, I saw the requirement after PRing. It should be OK now.

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 18, 2024
Comment thread system/Common.php Outdated

@ddevsr ddevsr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests for data array? recursive and non-recursive

@ddevsr ddevsr added the tests needed Pull requests that need tests label Mar 19, 2024
@kenjis

kenjis commented Mar 19, 2024

Copy link
Copy Markdown
Member

The following php-cs-fixer error is not related to this PR.

Files that were not fixed due to errors reported during fixing:
   1) /home/runner/work/CodeIgniter4/CodeIgniter4/app/Config/Logger.php

https://github.com/codeigniter4/CodeIgniter4/actions/runs/8327332402/job/22819194579?pr=8633

I sent a bug report: PHP-CS-Fixer/PHP-CS-Fixer#7896

@Cleric-K

Copy link
Copy Markdown
Contributor Author

Can you add tests for data array? recursive and non-recursive

Can you point me to an existing test for a function that I can use as an example? I'm not familiar with the CI project structure.

@kenjis

kenjis commented Mar 19, 2024

Copy link
Copy Markdown
Member

See

public function testEscapeWithDifferentEncodings(): void
{
$this->assertSame('&lt;x', esc('<x', 'html', 'utf-8'));
$this->assertSame('&lt;x', esc('<x', 'html', 'iso-8859-1'));
$this->assertSame('&lt;x', esc('<x', 'html', 'windows-1251'));
}
public function testEscapeBadContext(): void
{
$this->expectException('InvalidArgumentException');
esc(['width' => '800', 'height' => '600'], 'bogus');
}
public function testEscapeBadContextZero(): void
{
$this->expectException('InvalidArgumentException');
esc('<script>', '0');
}

@Cleric-K

Copy link
Copy Markdown
Contributor Author

I added two tests. I don't have much experience with unit tests so please tell me if it seems right.

@kenjis kenjis removed the tests needed Pull requests that need tests label Mar 20, 2024
@kenjis

kenjis commented Mar 20, 2024

Copy link
Copy Markdown
Member

@Cleric-K Please fix the coding style.
composer cs-fix will fix the errors.
See https://github.com/codeigniter4/CodeIgniter4/actions/runs/8343103783/job/22862006485?pr=8633

Comment thread system/ThirdParty/Escaper/Escaper.php Outdated

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kenjis
kenjis merged commit da4861c into codeigniter4:develop Mar 22, 2024
@kenjis

kenjis commented Mar 22, 2024

Copy link
Copy Markdown
Member

@Cleric-K Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: esc(..., 'raw') fails on recursive data structures

3 participants