Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes strpos for php 7.3 #1915

Merged
merged 10 commits into from
Feb 22, 2019
Merged

Fixes strpos for php 7.3 #1915

merged 10 commits into from
Feb 22, 2019

Conversation

palmiak
Copy link
Collaborator

@palmiak palmiak commented Jan 30, 2019

Ticket: #1899

Issue

Problem with php 7.3 and strpos - described in ticket.

Solution

As stated in PHP docs:

If needle is not a string, it is converted to an integer and applied as the ordinal value of a character. This behavior is deprecated as of PHP 7.3.0, and relying on it is highly discouraged. Depending on the intended behavior, the needle should either be explicitly cast to string, or an explicit call to chr() should be performed. 

so I added the chr function.

Impact

None.

Usage Changes

None

Considerations

Testing

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 94.233% when pulling 46bf63f on palmiak:#1899 into 3beeae4 on timber:master.

@coveralls
Copy link

coveralls commented Jan 30, 2019

Coverage Status

Coverage increased (+0.1%) to 94.267% when pulling d85bc0a on palmiak:#1899 into 4c9584d on timber:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 94.233% when pulling 46bf63f on palmiak:#1899 into 3beeae4 on timber:master.

@fluff
Copy link

fluff commented Feb 7, 2019

Adding chr() does not seem right at all. Please see my comment in #1899 (comment)

@palmiak
Copy link
Collaborator Author

palmiak commented Feb 18, 2019

Just leaving this here to test later:

protected static function is_in_theme_dir( $path ) {
    $root = realpath(get_stylesheet_directory());

    if ( false === $root ) {
        return false;
    }

    if ( 0 === strpos($path, (string)$root) ) {
        return true;
    } else {
        return false;
    }
}

@@ -328,7 +328,7 @@ protected static function process_delete_generated_files( $filename, $ext, $dir,
*/
public static function get_server_location( $url ) {
// if we're already an absolute dir, just return
if ( 0 === strpos($url, ABSPATH) ) {
if ( 0 === strpos($url, chr(ABSPATH)) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this chr() should be here either?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw that too - pushed fix already. Thanks.

@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #1915 into master will decrease coverage by 1.35%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1915      +/-   ##
============================================
- Coverage     95.01%   93.65%   -1.36%     
- Complexity     1553     1554       +1     
============================================
  Files            48       48              
  Lines          3669     3672       +3     
============================================
- Hits           3486     3439      -47     
- Misses          183      233      +50
Impacted Files Coverage Δ Complexity Δ
lib/ImageHelper.php 95.57% <83.33%> (-0.7%) 89 <0> (+1)
lib/Site.php 51.19% <0%> (-39.29%) 23% <0%> (ø)
lib/Cache/Cleaner.php 68.42% <0%> (-31.58%) 6% <0%> (ø)
lib/Timber.php 87.87% <0%> (-8.09%) 32% <0%> (ø)
lib/Loader.php 95.51% <0%> (-0.65%) 67% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11c93a8...ca8ee01. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #1915 into master will decrease coverage by 0.02%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1915      +/-   ##
============================================
- Coverage     95.01%   94.98%   -0.03%     
- Complexity     1554     1555       +1     
============================================
  Files            48       48              
  Lines          3669     3672       +3     
============================================
+ Hits           3486     3488       +2     
- Misses          183      184       +1
Impacted Files Coverage Δ Complexity Δ
lib/ImageHelper.php 95.94% <80%> (-0.33%) 89 <0> (+1)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c9584d...d85bc0a. Read the comment docs.

@palmiak
Copy link
Collaborator Author

palmiak commented Feb 21, 2019

@jarednova I'm looking at the Travis test results and don't quite understand all the errors. I think that the problem is mostly caused by WP 4.7.9 + PHP 7.3 not the fix itself.

Do you have any idea how to fix those bugs?

@jarednova
Copy link
Member

@palmiak according to this post on WordPress and PHP 7.3, they aimed for full 7.3 support in the next release, which at that time would have been 5.0.

Because 4.7.x gets us to +80% in WP version support I'd like to take an honest whack at this one and see what happens if we go up to 4.7.12 for the 4.7 version we test against. Let's see what that does ....

@palmiak
Copy link
Collaborator Author

palmiak commented Feb 21, 2019

@jarednova 4.7.12 didn't changed anything 😞 We can assume that 4.7.* won't be compatible with 7.3.

Can we remove just the tests for 4.7 + php 7.3 because they'll always give us error.

@jarednova
Copy link
Member

Yeah, it appears the WordPress gods have not smiled upon us. I'd like to still test against a "lower" WP version so I just upped that to 4.8.x to see if that makes Travis happy

@jarednova
Copy link
Member

Ended up going back down to WP 4.7.12 but excluding PHP 7.3 for that combo. All tests are good now!

@jarednova jarednova merged commit 4dcd7dd into timber:master Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants