Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memory_limit option is not properly validate when passed via MemoryOptions::__construct or set via MemoryOptions#setMemoryLimit #54

Open
leuchtdiode opened this issue Mar 5, 2024 · 5 comments · May be fixed by #56
Assignees
Labels
Bug Something isn't working
Milestone

Comments

@leuchtdiode
Copy link

Hi,

I'm debugging why my given memory limit is not being applied in the adapter. Looks like there is a bug in the MemoryOptions. I think preg_match in this line

if (! preg_match('/(\-?\d+)\s*(\w*)/', ini_get('memory_limit'), $matches)) {

should use the parameter $value instead of ini_get('memory_limit')?

BR,
Alex

@leuchtdiode leuchtdiode changed the title Bug: Wrong value used Bug: Wrong parameter used Mar 5, 2024
@Ocramius
Copy link
Member

Ocramius commented Mar 5, 2024

I'm debugging why my given memory limit is not being applied in the adapter.

Would you be able to write a small integration test using ini_set() and @runInSeparateProcess, perhaps?

@Ocramius Ocramius added Bug Something isn't working Unit Test Needed labels Mar 5, 2024
@leuchtdiode
Copy link
Author

I'm debugging why my given memory limit is not being applied in the adapter.

Would you be able to write a small integration test using ini_set() and @runInSeparateProcess, perhaps?

I will try to find some time the next days to write some tests.

@Xerkus
Copy link
Member

Xerkus commented Mar 5, 2024

Introduced in time immaterial by b87cc74

There are no tests whatsoever for options object.
@leuchtdiode It would be great if you introduce them.

For this problem ini_set/ini_get is not needed. Test that:

  • call to setMemoryLimit() with invalid value like 'M128' produces exception with message containing "Invalid memory limit".
  • call to setMemoryLimit() actually sets it.

@leuchtdiode
Copy link
Author

leuchtdiode commented Mar 17, 2024

I just created a pull request, but there are some issues with my formatting. Do you have a code style XML somewhere for PHPStorm which I can use?

Furthermore I added these tests:

[ 'M256', 256 ] [ 'G1024', 1024 ]

as valid ones. In my opinion they should raise an InvalidArgumentException as well. What do you think? Which values do we want to allow? Only integers and "G", "M" and "k" as suffix, right? If so, I could adapt the regex and the tests.

Also I added to return "null" when "null" is given to normalize, because it broke the already existing test testOptionsFluentInterface from the common base test class.

@Ocramius
Copy link
Member

Do you have a code style XML somewhere for PHPStorm which I can use?

There's a PHPCS configuration at the root of the repository:

<?xml version="1.0"?>
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="vendor/squizlabs/php_codesniffer/phpcs.xsd">
<arg name="basepath" value="."/>
<arg name="cache" value=".phpcs-cache"/>
<arg name="colors"/>
<arg name="extensions" value="php"/>
<arg name="parallel" value="80"/>
<!-- Show progress -->
<arg value="p"/>
<!-- Paths to check -->
<file>src</file>
<file>test</file>
<!-- Include all rules from the Laminas Coding Standard -->
<rule ref="LaminasCodingStandard"/>
</ruleset>

as valid ones. In my opinion they should raise an InvalidArgumentException as well. What do you think? Which values do we want to allow? Only integers and "G", "M" and "k" as suffix, right? If so, I could adapt the regex and the tests.

Worth discussing on the PR directly :)

@boesing boesing linked a pull request Mar 21, 2024 that will close this issue
@boesing boesing changed the title Bug: Wrong parameter used Memory Limit option is not properly validate when passed via __construct or set via MemoryOptions#setMemoryLimit Mar 22, 2024
@boesing boesing changed the title Memory Limit option is not properly validate when passed via __construct or set via MemoryOptions#setMemoryLimit Memory Limit option is not properly validate when passed via Memory::__construct or set via MemoryOptions#setMemoryLimit Mar 22, 2024
@boesing boesing changed the title Memory Limit option is not properly validate when passed via Memory::__construct or set via MemoryOptions#setMemoryLimit memory_limit option is not properly validate when passed via MemoryOptions::__construct or set via MemoryOptions#setMemoryLimit Mar 22, 2024
@boesing boesing self-assigned this Mar 22, 2024
@boesing boesing added this to the 2.3.1 milestone Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants