WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 4 months ago

#45435 new enhancement

Port Gutenberg's `removep` Function to Php

Reported by: 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:
PR Number:

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 11 months ago.

Download all attachments as: .zip

Change History (11)

#1 @gaveline
11 months ago

I'm working on it right now.

#2 @gaveline
11 months 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
11 months 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
11 months ago

Hi,

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

#5 @gaveline
11 months 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
11 months ago

Hi @gaveline

I agree. Makes sense.

#7 @gaveline
11 months 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
11 months 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
11 months 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
4 months 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.

Note: See TracTickets for help on using tickets.