Page MenuHomeIn-Portal Phabricator

INP-1729 Use "preg_replace_callback" instead of "preg_replace" with "e" modifier
ClosedPublic

Authored by erik on Nov 8 2017, 11:27 AM.

Details

Summary

Removed deprecated e-modifier from preg_replace calls.

Test Plan
  • general note:
    1. if possible, repeat all tests with 5.3 and 5.6+ php versions
    2. after testing with 5.6+ php version additionally confirm, that no records were added to the System Log, because usage of "e" modifier, if not fixed, produces notice record in the System Log
  • in Admin Console:
    1. go to ToolsQuery Database section
    2. run SQL UPDATE inp_EmailTemplates SET l1_HtmlBody = CONCAT(l1_HtmlBody, '®') WHERE TemplateName = 'TOPIC.ADD' AND Type = 1; to add HTML representation of registered trade mark sign, to the end of particular e-mail template
    3. go to User ManagementUsers section
    4. create new user
  • on Front-End:
    1. login with newly created user
    2. click on Forums top menu entry
    3. click on the "New Topic" link in the sidebar's "Action Box" section
    4. fill all required fields and in the Message Body field type before [B]bold[/B] after [CODE]Test [B]Co[/B]de[/CODE] before [B]bold[/B] [URL HREF=http://www.google.com]the link[/URL] after text
    5. uncheck "Disable BBCodes" checkbox
    6. submit form (click "Create" button)
    7. go to "My Topics" page
    8. click on the "Details" link of just created topic
    9. confirm that BBCodes outside of [CODE]...[/CODE] block were evaluated (means text is bold partially and link is clickable)
    10. confirm that BBCodes inside of [CODE]...[/CODE] block weren't evaluated (the BBCodes are displayed with [ replaced with < and ] replaced with >)
  • in Admin Console:
    1. go to the Website & ContentLogs & ReportsEmail Log section
    2. confirm that TOPIC.ADD Admin e-mail was sent
    3. View details of TOPIC.ADD Admin e-mail
    4. confirm, that "Text Version" value has copyright symbol at the end

Diff Detail

Repository
rINP In-Portal
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

erik created this revision.Nov 8 2017, 11:27 AM
erik edited the test plan for this revision. (Show Details)Nov 8 2017, 11:29 AM
erik edited the test plan for this revision. (Show Details)
alex requested changes to this revision.Nov 8 2017, 2:08 PM

Also in test plan instead of asking to add &#169; to PHP code ask to add it directly to email template body, that will be sent. This is how it works in real life.

core/kernel/utility/email_send.php
714 ↗(On Diff #817)

This doesn't look like something, that will work on PHP 5.3, because late static binding (the stuff that makes $this work in callbacks) was introduced only in PHP 5.4.

Have you tested (debugged) this on PHP 5.3 as well and no notices were triggered?

You can use approach like this:

$that = $this;
... function ($matches) use ($that) {
    return $that->_safeCharEncode(...
modules/in-bulletin/units/helpers/post_helper.php
25–30 ↗(On Diff #817)

Remove. See below why.

300 ↗(On Diff #817)

Instead of defining class property use same closure approach as in email_send.php file.

301 ↗(On Diff #817)

This will work, but would trigger a notice on each use, because replaceCodeBBCode isn't a string.

411 ↗(On Diff #817)

Please replace protected with public.


All callbacks must have public visibility even if they're used only within same class.

This revision now requires changes to proceed.Nov 8 2017, 2:08 PM
erik added inline comments.Nov 21 2017, 5:48 AM
core/kernel/utility/email_send.php
714 ↗(On Diff #817)
  1. yes, I tested.
  2. late static binding is appeared from 5.3.0 - http://php.net/manual/en/language.oop5.late-static-bindings.php
erik added a comment.Nov 21 2017, 6:01 AM

Today written code, but not tested.

erik edited the test plan for this revision. (Show Details)Nov 23 2017, 5:22 AM
erik edited the test plan for this revision. (Show Details)
erik updated this revision to Diff 818.Nov 23 2017, 5:33 AM
erik edited edge metadata.

Made all requested fixes.

alex requested changes to this revision.Nov 23 2017, 12:17 PM
alex added inline comments.
modules/in-bulletin/units/helpers/post_helper.php
410 ↗(On Diff #818)

Since the $matches argument is an array it must have array typehint (the array $matches instead of just $matches).

This revision now requires changes to proceed.Nov 23 2017, 12:17 PM
erik updated this revision to Diff 819.Nov 24 2017, 5:41 AM
erik edited edge metadata.

Made requested change and fixed some bugs.

erik edited the test plan for this revision. (Show Details)Nov 24 2017, 5:42 AM
alex added inline comments.May 7 2018, 10:01 AM
core/kernel/utility/email_send.php
714 ↗(On Diff #817)

Maybe it's called differently. I've tested that on PHP 5.3 attempt to use $this in closures would fail: https://3v4l.org/snQZe

alex edited the test plan for this revision. (Show Details)May 7 2018, 10:22 AM
alex edited the test plan for this revision. (Show Details)May 7 2018, 10:31 AM
alex edited the test plan for this revision. (Show Details)
alex requested changes to this revision.May 7 2018, 10:33 AM
alex added inline comments.
modules/in-bulletin/units/helpers/post_helper.php
282 ↗(On Diff #819)
  1. Apply same approach as with replaceCodeBBCode method call below.
  2. See if preg_replace is being used with e modified anywhere else and fix these usages as well.

Not everything was fixed.

This revision now requires changes to proceed.May 7 2018, 10:33 AM
alex added a comment.May 7 2018, 10:35 AM

Also in test plan:

  1. specify, that PHP 5.6+ needs to be used
  2. confirm, that no records were added to the System Log
erik updated this revision to Diff 853.May 8 2018, 7:11 AM
erik edited edge metadata.

Removed e-modifier in second place. Applied advanced test plan.

alex added a comment.May 9 2018, 4:00 AM
In D328#6702, @erik wrote:

Removed e-modifier in second place. Applied advanced test plan.

I'm not seeing any changes to test plan. Have you saved them?

alex requested changes to this revision.May 9 2018, 4:04 AM
alex added inline comments.
modules/in-bulletin/units/helpers/post_helper.php
278–289 ↗(On Diff #853)

Something wrong with indentation here.

299–308 ↗(On Diff #853)

Something wrong with indentation here.

421–422 ↗(On Diff #853)

Please revert.


No need to change non-related code for CS issues only.

This revision now requires changes to proceed.May 9 2018, 4:04 AM
erik edited the test plan for this revision. (Show Details)May 9 2018, 6:46 AM
erik edited the test plan for this revision. (Show Details)
erik updated this revision to Diff 854.May 9 2018, 7:11 AM
erik edited edge metadata.

Removed excessive code changes, not related to this task.

alex edited the test plan for this revision. (Show Details)May 16 2018, 4:32 AM
alex accepted this revision.May 16 2018, 4:43 AM
This revision is now accepted and ready to land.May 16 2018, 4:43 AM
This revision was landed with ongoing or failed builds.Mar 15 2021, 3:38 AM
This revision was automatically updated to reflect the committed changes.