Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#45435 new enhancement

Port Gutenberg's `removep` Function to Php

Reported by: conner_bw's profile conner_bw Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9.8
Component: Editor Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Gutenberg has autop and removep functions, code is here:

https://github.com/WordPress/gutenberg/tree/master/packages/autop

autop in JS is equivalent to wpautop in PHP.

There is no removep or wpremovep in PHP.

I have often needed it. Please port it into WordPress for PHP developers.

Thank you for your consideration.

Attachments (1)

45435-wpremovep.diff (15.7 KB) - added by gaveline 5 years ago.

Download all attachments as: .zip

Change History (12)

#1 @gaveline
5 years ago

I'm working on it right now.

#2 @gaveline
5 years ago

  • Keywords has-patch has-unit-tests added

Here's my diff file attached to the ticket.
I nearly write line toi line code from JS file and test each case.
Since implementation may be refactored and enhanced, tests will simplify the task.

Changes

 src/wp-includes/formatting.php             | 139 ++++++++
 tests/phpunit/tests/formatting/Removep.php | 351 +++++++++++++++++++++
 2 files changed, 490 insertions(+)
 create mode 100644 tests/phpunit/tests/formatting/Removep.php

Running test for this ticket :
phpunit --group 45435

.............                                                     13 / 13 (100%)

Time: 10.98 seconds, Memory: 144.00MB

OK (13 tests, 13 assertions)

#3 @conner_bw
5 years ago

Current diff fails this test:

<?php
        public function test_reverse_wpautop() {
                $raw = <<< RAW
Hi there!

How's it going?

[shortcode id="1"]Fake[/shortcode]

Ok Bye!

<pre>My
line
breaks
must
remain
</pre>
RAW;

                $var = wpautop( $raw );
                $var = wpremovep( $var );

                $this->assertEquals( trim( $raw ), trim( $var ) );
        }

Expected:

[shortcode id="1"]Fake[/shortcode]\n
\n
Ok Bye!\n
\n
<pre>My\n

Actual:

[shortcode id="1"]Fake[/shortcode]\n
\n
Ok Bye!\n
<pre>My\n

(I have this test because we wrote our own reverse_wpautop. We'd much rather be using a WordPress core function but we also would need it to pass our two tests...)

#4 @gaveline
5 years ago

Hi,

Thanks for your feedback.
I'll take a look at it.

#5 @gaveline
5 years ago

Well, maybe your implementation is better than mine, but the original ticket was to expose a function wpremovep, similar to gutenberg one.

When i try your test in the JS version i got this :

Lets init a raw value :

let raw = `Hi there!

How's it going?

[shortcode id=\"1\"]Fake[/shortcode]

Ok Bye!

<pre>My
line
breaks
must
remain
</pre>`

And ask to Gutenberg to

let var = autop(raw)

After this step, var equals to :

<p>Hi there!</p>
<p>How's it going?</p>
<p>[shortcode id="1"]Fake[/shortcode]</p>
<p>Ok Bye!</p>
<pre>My
line
breaks
must
remain
</pre>

Than we do that

var = removep(var)

After this step, var equald to

Hi there!

How's it going?

[shortcode id="1"]Fake[/shortcode]

Ok Bye!
<pre>My
line
breaks
must
remain
</pre>

Which fits with my implementation too.

I suggest we merge with the strict same implementation and then open a new ticket for fixing both implementations (JS and PHP).

I think it much consistent to have the same returns in JS and PHP, event if they have a little weird behavior.

#6 @conner_bw
5 years ago

Hi @gaveline

I agree. Makes sense.

#7 @gaveline
5 years ago

Great @conner_bw :)

Now, as it's my first patch, I don't know what is the next step for a PR.

And after looking at your implementation, your big preg pattern is a masterpiece.
We may want to implement it when refactoring my poor "line by line translation" :)
(I Hope your pattern will fit all the tests then)

Regards.

#8 @conner_bw
5 years ago

Now, as it's my first patch, I don't know what is the next step for a PR.

Ha. Me neither. Hopefully someone from the WordPress team is reading.

#9 @ayeshrajans
5 years ago

You are right we'd have to wait for someone responsible for the component to take a look and provide their feedback.
Certain straight forward bug fixes are accepted and merged quickly, but changes like this will have a discussion and you may need to monitor the issue for back and forth suggestions and changes.

Congratulations on your first patch!

#10 @apedog
5 years ago

Is there any chance to see this in 5.3?
I'm very much in need of wpremovep in PHP. ATM I'm polyfilling, but this should be in WordPress core.

#11 @noisysocks
3 years ago

Thanks for the patch!

As a rule I'm always a little wary of adding new APIs to Core that aren't used in Core anywhere. Could you explain a little about your use case for wpremovep? Are there existing alternatives? What are the pros and cons of wpremovep versus any existing alternatives?

Note: See TracTickets for help on using tickets.