I had this error come up repeatedly in /sites/all/modules/ubercart/uc_store/uc_store.module on line 2042. after I updated to Ubercart 5.x-1.10.

So I went into the uc_store.module and changed the following lines:

Line 2040:
$match = array('/^
/', '/
$/', '/
(\s*|[\s*
\s*]+)
/', '/

/', '`
, N/A`');

to
$match = array('/^
/', '/
$/', '/
(\s*|[\s*
\s*]+)
/', '/

/', '`
, N/A`');

Line 2041:
$replace = array('', '', '
', '
', '', '');

to
$replace = array('', '', '
', '
', '', '');

Don't quite know if I did the right thing but the error seems to have disappeared.

CommentFileSizeAuthor
#6 880908.patch919 bytesTR
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

armetrix’s picture

Status: Active » Needs review
kdebaas’s picture

Could you post a patch, or wrap your changes in code tags? It's impossible to see what it is that you have changed exactly. But it looks like the changes of #421366: Invalid XHTML tags needed to escape some of the forward slashes.

ledom’s picture

Same issue here.
Solved by using old version (1.9) of lines 2040 and 2041

Old version (1.9)

  $match = array('/^<br>/', '/<br>$/', '/<br>(\s*|[\s*<br>\s*]+)<br>/', '/<br><br>/', '`<br>, N/A`');
  $replace = array('', '', '<br>', '<br>', '', '');

New (1.10)

  $match = array('/^<br />/', '/<br />$/', '/<br />(\s*|[\s*<br />\s*]+)<br />/', '/<br /><br />/', '`<br />, N/A`');
  $replace = array('', '', '<br />', '<br />', '', '');
TR’s picture

(Please don't use "needs review" unless there's an actual patch to review!)

Here's the patch. Please test.

Be aware that when Drupal 5 becomes unsupported, Ubercart 1.x will also be unsupported. That's probably going to happen in the next month. So I advise you to begin upgrading as soon as possible.

Druid’s picture

Ref: #3

If strings such as '/^<br />/' are being passed to preg functions as search patterns, aren't you going to have problems with the middle "/" ending the pattern? That's what the original error message is stating. Wouldn't it have to be '/^<br \/>/' or '#^<br />#'? You are using / as a pattern delimiter, so any / within the pattern would need to be escaped.

P.S. Is it definitely <br /> in all cases, or could there be <br/> in some cases? Either is valid XHTML — perhaps the search pattern should be <br */> (0 or more spaces before /) or even <br */?> (optional self-closing)?

TR’s picture

FileSize
919 bytes

Forgot to attach the patch ... patch changes the Ubercart 1.x version of uc_store.module to use the same pattern matching as the Ubercart 2.x version.

kdebaas’s picture

Applied patch, and watchdog messages went away. Would you like me to test anything else?

whoey’s picture

same here, patch seems to have sorted the issue Cheers!

TR’s picture

Status: Needs review » Fixed

OK, patch committed.

j0rd’s picture

Same issue. Applied patch. Problem Solved.

Many businesses won't have the man hours or funds upgrade right away, so at least implementing simple bug fixes back into the tree would be nice.

I personally just took on a Ubercart 1.x / Drupal 5.x project...but I think I'll be holding out on an upgrade to D6/UC2 and instead just wait a couple extra months and jump right to D7 / UC3. That makes more economic sense for most companies in the long run I think.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

frost’s picture

@j0rd:
i think that if you're doing a "normal" upgrade, you'll still have to upgrade to 6 along the way to 7. some people might just rebuild the site starting in 7, but i suppose it's a balance of the amount of work you'll need to do in either case. upgrades aren't always problem free ;-). i'm just now upgrading a 5.x site with ubercart to 6.x even tho i know there'll be a 7.x upgrade sometime in the next year (depending on when all the dependent modules are available for 7)

shaven’s picture

Druid wins!!! He is absolutely right. Here is what it needs to look like:

$match = array('/^<br \/>/', '/<br \/>$/', '/<br \/>(\s*|[\s*<br \/>\s*]+)<br \/>/', '/<br \/><br \/>/', '`<br \/>, N/A`');
$replace = array('', '', '<br />', '<br />', '', '');

Please update the patch.

kingandy’s picture

@shaven: The '/' mid-string token was the cause of the initial issue ("Unknown modifier '>'" - the '>' being the '>' character after the '/'). The patch surrounds the '/' with brackets ('( /)?'), apparently in order to make it optional; this appears to have the happy side effect of fixing the preg_replace error, as the parser does not consider a '/' inside a bracketed clause to denote the end of the regexp. The patch works correctly.

Druid’s picture

Re: #13 and #14

If it works, it works. I would point out that you don't have to use / as the delimiter, and escape it within the pattern. The $match could have been
$match = array('#^<br />#', '#<br />$#', '#<br />(\s*|[\s*<br />\s*]+)<br />#', '#<br /><br />#', '`<br />, N/A`');
(not certain about that last element -- are backticks just being used as the PREG delimiter?). As <br/> is also legitimate, you may want to allow 0 or more spaces between the br and the /. There's also the issue of plain <br> to consider.

I think the Perl Regexp implementers really missed the boat on this one. They could have scanned left-to-right for the delimiter (pick up /), then scanned right-to-left from the right end of the string to pick up the flags and find the closing delimiter. This would leave everything in between as the pattern, without having to worry about escaping the delimiter which happens to be in the pattern. They might have even dropped the explicit delimiters (as ereg did) and add them as a separate argument, or reserved a certain character as an end-of-pattern separator to mark that flags are coming up at the end of the string. In that case, the separator would have to be escaped if it occurred in the pattern, so maybe nothing's gained in the end.

preg_match('/<br */?>/i',...  scan /, scan i scan /, pattern is <br */?>
preg_match('i', '<br */?>',...  ereg style pattern, with empty '' if no flags
preg_match('<br */?>%i',...  any % in the pattern would have to be \%

etc. Any way, it's still incompatible with ereg, so it's debatable as to whether anything is gained with this syntax.

resting’s picture

i downloaded 5.x-1.10 released on 2010-Aug-11. Had the same error.
Applied this patch and the error is gone.

Can it be committed to the main package?

Drupal Centric’s picture

Patch also worked for me using 5.x-1.10, thanks.