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

Replacing simple preg calls with less expensive alternates #341

Closed
wants to merge 6 commits into from
Closed

Replacing simple preg calls with less expensive alternates #341

wants to merge 6 commits into from

Conversation

parhamr
Copy link

@parhamr parhamr commented Aug 21, 2013

Changes

  • Replaces simple preg_match calls with equivalent substr, strpos, and/or strlen checks
  • Replaces simple preg_split calls with equivalent explode operations

Testing

  • Code review, please
  • No functional changes

@verklov
Copy link
Contributor

verklov commented Nov 7, 2013

Hello parhamr,
Sorry for the delay with the response.
Thank you for your contribution! Our team will review your pull request and respond as soon as our analysis is complete.

@orlangur
Copy link
Contributor

preg_split -> explode change seems to be reasonable, but combinations of substr/strpos/strlen mess up the code.

Did you measure the gain within real application? Referring to this comment: any code change without evidence of performance improvement is premature optimization.

@parhamr
Copy link
Author

parhamr commented Nov 20, 2013

I’m not sure what you mean by mess up the code; those function names describe exactly their intent and purpose.

No, I did not measure any changes. This is a matter of PHP best practices and code style and it’s up to your organization to determine what standards and quality are acceptable for your core product.

@IvanChepurnyi
Copy link
Contributor

@orlangur Please check official PHP documentation about preg_match. There is a good note from PHP core team:

Tip
Do not use preg_match() if you only want to check if one string is contained in another string. Use strpos() or strstr() instead as they will be faster.

magento-team added a commit that referenced this pull request Dec 14, 2013
* Fixed bugs:
  * Fixed placing order with PayPal Payments Advanced and Payflow Link
  * Fixed losing previously assigned categories after saving the product with changed category selector field
  * Fixed losing of a newly created category assignment after variations generation during Configurable product or Gift Card creation
  * Fixed the error in order placement with Recurring profile payment
* GitHub requests:
  * [#299](#299) -- Fix for issue Refactor Mage_Rating_Model_Resource_Rating_Collection
  * [#341](#341) -- Replacing simple preg calls with less expensive alternates
* Modularity improvements:
  * Layout page type config moved to library
  * Design loader moved to library
  * Theme label moved to library
* Themes update:
  * Reduced amount of templates and layouts in magento_plushe theme
  * Responsive design improvements
* Integrity improvements:
  * Covered all Magento classes with argument sequence validator
  * Added arguments type duplication validator
* Implemented API Integration UX flows:
  * Ability to create and edit API Integrations
  * Ability to delete API integrations that were not created using configuration files
* Removed System REST menu item and all associated UX flows:
  * Users, Roles, and Webhook Subscriptions sub-menu items were removed
* Removed the Webhook module
@verklov
Copy link
Contributor

verklov commented Dec 14, 2013

parhamr,
Our team has processed your pull request. The code is available in the version dev56 recently released.

Thank you for your contribution! Looking forward for your new contributions.

@verklov verklov closed this Dec 14, 2013
magento-team pushed a commit that referenced this pull request Jun 12, 2015
[API] Sprint 49 – Test Coverage and Bugs
magento-engcom-team added a commit that referenced this pull request Feb 11, 2019
 - Merge Pull Request magento/graphql-ce#341 from kisroman/graphql-ce:graphQl-255
 - Merged commits:
   1. 1bbeb21
magento-engcom-team added a commit that referenced this pull request Feb 11, 2019
 - Merge Pull Request magento/graphql-ce#341 from kisroman/graphql-ce:graphQl-255
 - Merged commits:
   1. 1bbeb21
magento-engcom-team pushed a commit that referenced this pull request Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants