Make WordPress Core

Opened 5 years ago

Last modified 7 days ago

#46550 new defect (bug)

Uncaught TypeError: setcookie() expects parameter 5 to be string, bool given in...

Reported by: kmvan's profile kmvan Owned by:
Milestone: 6.6 Priority: normal
Severity: minor Version: 5.2
Component: Networks and Sites Keywords: has-patch has-testing-info early needs-testing
Focuses: coding-standards Cc:

Description

https://github.com/WordPress/WordPress/blob/5e62e6b2034516c0bb366f808673752030d2d2b7/wp-includes/default-constants.php#L303

<?php
        /**
         * @since 2.0.0
         */
        if ( ! defined( 'COOKIE_DOMAIN' ) ) {
                define( 'COOKIE_DOMAIN', false ); // The value maybe '', not boolean
        }

Change History (19)

#1 @desrosj
5 years ago

  • Keywords reporter-feedback added; needs-patch removed
  • Severity changed from critical to normal

Hi @kmvan,

Your ticket is not very detailed. It's impossible to look into this further without more information.

Are you running a single site install or a multisite install? Where is the call to setcookie()? Is it in WordPress Core? A plugin? A theme? Please provide more details so that others can reproduce your issue.

#2 @kmvan
5 years ago

Hello @desrosj,

The function setcookie() args: http://php.net/setcookie

setcookie ( string $name [, string $value = "" [, int $expires = 0 [, string $path = "" [, string $domain = "" [, bool $secure = FALSE [, bool $httponly = FALSE ]]]]]] ) : bool

The 5th parameter type is string, but the default constant COOKIE_DOMAIN type is boolean.

<?php

// So when I code below in my theme/plugin:
declare(strict_types = 1);

\setcookie(
    'name',
    'value',
    \time() + \YEAR_IN_SECONDS,
    \COOKIEPATH,
    \COOKIE_DOMAIN // This is not string
//    \is_bool(\COOKIE_DOMAIN) ? \COOKIE_DOMAIN : '' // I need to code on this way XD
);

So I think the default value shall be define( 'COOKIE_DOMAIN', '' );.

This ticket was mentioned in PR #4952 on WordPress/wordpress-develop by @rajinsharwar.


7 months ago
#3

  • Keywords has-patch added

Setting the default value of COOKIE_DOMAIN to a null value

Trac ticket: https://core.trac.wordpress.org/ticket/46550

#4 @rajinsharwar
7 months ago

#58959 was marked as a duplicate.

#5 @rajinsharwar
7 months ago

  • Component changed from Bootstrap/Load to Networks and Sites
  • Focuses coding-standards php-compatibility added
  • Severity changed from normal to major

Yes, it was causing a Fatal error when the default value of the COOKIE_DOAMIN was used in any external PHP script. So, set the default value of the constant to null instead of false.

cc @cybr @johnbillion

#6 @Cybr
7 months ago

It causes no warnings nor errors in PHP unless strict typing is declared: https://3v4l.org/dEQk2.

However, to reclarify: setcookie() expects a string for $domain. false does nothing for it.

Nothing in WP checks against false, nor would a cookie have a boolean. This change should be fairly inconspicuous and ensures future compatibility, seeing how PHP is strictly typing internal functions.

It caught my eye, and I thought I'd mention it in Slack for an easy fix.

Last edited 7 months ago by Cybr (previous) (diff)

#7 @jrf
7 months ago

  • Keywords needs-testing added; reporter-feedback removed
  • Milestone changed from Awaiting Review to 6.4
  • Severity changed from major to minor

It causes no warnings nor errors in PHP unless strict typing is declared: https://3v4l.org/dEQk2.

Agreed and using strict_types = 1 in the context of WP is generally not the greatest idea as WP is as loosely as possible, so this would easily cause a mountain of errors.

Having said that, the proposed fix looks like a good improvement, though for reasons of synchronicity with PHP core, not for compatibility reasons.

Version 0, edited 7 months ago by jrf (next)

This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.


5 months ago

#9 @nicolefurlan
5 months ago

  • Keywords needs-testing-info added

@kmvan could you add testing instructions to this ticket? Thank you! :)

#10 follow-up: @kmvan
5 months ago

@nicolefurlan Sorry, this ticket is from five years ago. I no longer write programs and I don't have a computer. I'm sorry that I can't add unit tests. T_T

#11 in reply to: ↑ 10 @nicolefurlan
5 months ago

No worries @kmvan! I apologize I didn't notice that this was so long ago!

Can someone else please add testing instructions for this ticket so we can keep it moving along? Thank you!

#12 @oglekler
5 months ago

  • Keywords has-testing-info added; needs-testing-info removed

For testers:

<?php

 declare( strict_types = 1 ); // Add this to the beginning of the file

 setcookie(
    'name',
    'value',
    time() + YEAR_IN_SECONDS,
    COOKIEPATH,
    COOKIE_DOMAIN
  );

I tried and got the fatal error as described. The patch is solving the problem.

#13 @hellofromTonya
5 months ago

  • Focuses php-compatibility removed
  • Keywords changes-requested added; needs-testing removed

Agreed and using strict_types = 1 in the context of WP is generally not the greatest idea as WP is as loosy as possible, so this would easily cause a mountain of errors.

I agree with @jrf.

Updating the keywords:

  • Removing needs-testing as there is a testing report.
  • Adding changes-requested as there are documentation requested changes.

I wonder:

  • Why was false the default value?

I didn't find the why, but it was introduced in [2725] back 18 years ago in WP 2.0.

  • Are there any BC concerns of changing a Core constant's value?

A search of plugin usages did not reveal instances of specifically checking for a false value.

Having said that, the proposed fix looks like a good improvement, though for reasons of synchronicity with PHP core, not for compatibility reasons.

I agree with @jrf that this is not php-compatibility issue as the issue only happens when strict_types is turned on, which is not recommended. Having the fix could fall into "synchronicity with PHP core" as @jrf suggests.

That said, I do wonder if there are any BC concerns. Discovering instances in the wild that do check for false might be better served during the alpha phase, rather in the latest stage of beta cycle. Thoughts?

#14 @jrf
5 months ago

I do wonder if there are any BC concerns. Discovering instances in the wild that do check for false might be better served during the alpha phase, rather in the latest stage of beta cycle.

Well, my gut feeling says that issues resulting from the BC-break would be very rare and most likely would point to code which needs love anyway.

The typical usages of this constant will likely be:

  • Check if it is defined() and define() if not (= no value check).
  • Passing it to something like set_cookie() (= no value check).
  • Checking whether a cookie is valid for a given domain. This would involve a value check but most likely one where a cookie domain is compared against the constant. A cookie domain of false or '' will both result in a cookie being invalid when compared to an actual cookie domain, so this would not result in a behavioural change.

Having said that, I agree a late beta stage is probably not the best time to validate a gut feeling ;-)

It would be nice if the patch could get updated based on the previously given feedback so it is ready to be committed for 6.5 early.

#15 @hellofromTonya
5 months ago

  • Keywords early added
  • Milestone changed from 6.4 to 6.5

Thank you @jrf. Moving this ticket to 6.5 early, which gives it a longer soak time to validate if there are any BC concerns.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 weeks ago

#17 @kirasong
3 weeks ago

Chatted about this ticket in a triage session today, along with feedback from @chaion07.

@rajinsharwar are you interested in updating the docs in the PR?
As 6.5 is in Alpha, I think there's still an opportunity for this to land in the release if they are updated.

If not interested, then this ticket might be a good fit for a new contributor to update the docs in the PR / patch.

#18 @rajinsharwar
11 days ago

  • Keywords needs-testing added; changes-requested removed

Hi @mikeschroder, I tried to make the doc changes suggested. @hellofromTonya requesting feedback from you if this looks good now, or if we can land this in 6.5

#19 @swissspidy
7 days ago

  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.