[k2] fix preg_regplace_callback function #1477
Conversation
Shamzik
force-pushed
the
kshamazov/preg_regplace_callback
branch
4 times, most recently
from
9b0be9b to
e0af042
Compare
Shamzik
changed the title
Shamzik
force-pushed
the
kshamazov/preg_regplace_callback
branch
6 times, most recently
from
ee80ea3 to
86a4e75
Compare
Shamzik
force-pushed
the
kshamazov/preg_regplace_callback
branch
from
86a4e75 to
01b4486
Compare
…atched_as_null> dump_matches
|
Shouldn't this PR just fix the issue in |
apolyakov
left a comment
apolyakov
left a comment
There was a problem hiding this comment.
I think the current regex could benefit from some restructuring for better readability and maintainability. It’s a bit difficult to follow the logic as it stands
| co_return f$preg_replace(pattern, replacement, subject, limit, opt_count); | ||
| kphp::regex::details::Info regex_info{pattern, {subject.c_str(), subject.size()}, {}}; | ||
|
|
||
| if (!kphp::regex::details::valid_regex_flags(flags, kphp::regex::PREG_NO_FLAGS)) [[unlikely]] { |
There was a problem hiding this comment.
Are you sure these flags are enough?
There was a problem hiding this comment.
Zend says
flagscan be a combination of thePREG_OFFSET_CAPTUREandPREG_UNMATCHED_AS_NULLflags, which influence the format of the matches array.
The matches array is the array that will be passed to the callback function. The preg_replace_callback function in KPHP accepts the argument callable(string[] $x):string $callback . Thus, the callback function expects array to be an array of strings. However, this is only possible if all flags are unset. In the old KPHP runtime, the flags argument wasn't even defined in this function.
There was a problem hiding this comment.
It doesn't mean we need to drop support for flags. One day, the signature of the callback function may change, and that day will be not so easy to find a reason why we don't support these flags
| if (!kphp::regex::details::valid_regex_flags(flags, kphp::regex::PREG_NO_FLAGS)) [[unlikely]] { | ||
| co_return Optional<string>{}; | ||
| } | ||
| if (!compile_regex(regex_info)) [[unlikely]] { |
There was a problem hiding this comment.
I don't understand where it comes from... Please, specify a qualified name
| return true; | ||
| } | ||
|
|
||
| std::optional<array<mixed>> split_regex(kphp::regex::details::Info& regex_info, int64_t limit, bool no_empty, bool delim_capture, |
There was a problem hiding this comment.
Why is split_regex in anonymous namespace while compile_regex is in details one?
There was a problem hiding this comment.
Because it is not required to be accessed from the header, it is only used in the f$preg_split function.
| bool is_offset_capture, bool is_unmatched_as_null) noexcept; | ||
|
|
||
| template<std::invocable<array<string>> F> | ||
| coro::task<bool> replace_callback(Info& regex_info, F callback, uint64_t limit) noexcept { |
The goal is to fix a bug in the preg_regplace_callback function.
The error was related to the calculation of the replacement string. It should be calculated for each match. Previously, it was calculated only for the first match and used as the replacement string for every match.