Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

JRegistryFormatINI::stringToObject() preg_match key validation #1415

Closed
wants to merge 1 commit into from
Closed

JRegistryFormatINI::stringToObject() preg_match key validation #1415

wants to merge 1 commit into from

Conversation

phproberto
Copy link
Contributor

Fix an actually non-relevant but potential bug reported by eaxs in joomla-cms.

// Validate the key.
if (preg_match('/[^A-Z0-9_]/i', $key))
{
// Maybe throw exception?
continue;
}

Issue 267 reported by eaxs in joomla-cms
joomla/joomla-cms#267

@elinw
Copy link
Contributor

elinw commented Aug 6, 2012

Would you mind changing this title to something actually describing the change?

@phproberto
Copy link
Contributor Author

Sorry. Edited the title.

@pasamio
Copy link
Contributor

pasamio commented Aug 7, 2012

Do you mind putting in a unit test that shows this bug in action and can demonstrate it working properly?

@phproberto
Copy link
Contributor Author

As it's not a real bug can't be tested and doesn't worth to lose any time. Only have to see the if statement. Otherwise free to close the pull request.

@pasamio
Copy link
Contributor

pasamio commented Aug 10, 2012

I'm confused though, if it isn't a real bug what are we fixing? It sounds like it's a bug, all I'm asking for is a unit test to feed data to the function and replicate the error/prove it's fixed and ensure it doesn't get broken into the future.

@phproberto phproberto closed this Aug 10, 2012
@phproberto
Copy link
Contributor Author

If you see the if statement you can see that it doesn't do anything with or without the error. It's only a logical structure to complete in the future. Unit test has no sense as the if only contains a continue. It takes 2 min. to identify the problem.

We better forget this. If someday the logical structure is used the bug will appear and someone will fix it.

@pasamio
Copy link
Contributor

pasamio commented Aug 11, 2012

Then it should only take two minutes to build a unit test to verify it and ensure it works into the future.

@phproberto
Copy link
Contributor Author

Sure. Thank you for your time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants