Problem/Motivation

Tech press is covering this as "google retires FLoC":

https://blog.google/products/chrome/get-know-new-topics-api-privacy-sand...

In #3209628: Add Permissions-Policy header to block Google FLoC (and #3209976: Add Permissions-Policy header to block Google FLoC (D7)) Drupal added headers to block FLoC.

Are these (about to become) no longer necessary?

Should they be removed?

Steps to reproduce

Verify if the headers are now redundant.

Proposed resolution

...and remove them if so.

Remaining tasks

Remove FLoC headers from D9/10 and D7 (separate issue for D7).

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

tbc

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcdruid created an issue. See original summary.

antiorario’s picture

Or maybe replace it with the header to block the Topics API, because in the end it’s more of the same.

cilefen’s picture

Title: Google is abandonning FLoC - so remove the header? » Google is abandoning FLoC - so remove the header?
mcdruid’s picture

Visiting a Drupal site with the FLoC-blocking header in Chrome now emits a message in the console:

Error with Permissions-Policy header: Origin trial controlled feature not enabled: 'interest-cohort'.

Seems FLoC has defintely been replaced by Topics API (although that's still a "trial" as far as I can see):

https://blog.google/products/chrome/get-know-new-topics-api-privacy-sand...

I think we should remove the FLoC header, but would suggest that adding a replacement should be a separate issue.

mcdruid’s picture

Version: 10.0.x-dev » 9.5.x-dev
Status: Active » Needs review
FileSize
5.73 KB

Patch removing everything that was committed in #3209628: Add Permissions-Policy header to block Google FLoC from 9.5.x

Have I missed anything?

mcdruid’s picture

erm, not sure why that test was against 10.0.x when I changed version. Does this comment kick it back to 9.5.x?

mcdruid’s picture

Had to queue a 9.5.x test manually...

Spokje’s picture

Status: Needs review » Reviewed & tested by the community

LGTM

longwave’s picture

Just to note that the Topics API doc at https://github.com/patcg-individual-drafts/topics states:

A site can forbid topic calculation on a page via the following permission policy header:
Permissions-Policy: browsing-topics=()
Note: The old Permissions-Policy: interest-cohort=() from FLoC will also forbid topic calculation.

Unsure if we want to switch to this instead, or whether Topics API is less controversial so it should be up to individual sites via e.g. https://www.drupal.org/project/permissionspolicy

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We need to deprecate the setting then and tell users to clear it up. This will make @catch happy because we left the settings deprecation stuff in D10.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

miedward’s picture

I wasn't able to apply this patch to 9.4.8 release using composer-patch. Still in my headers though.

$ git apply -p1 --directory='web' 3260401-5.patch
error: patch failed: web/core/assets/scaffold/files/default.settings.php:593
error: web/core/assets/scaffold/files/default.settings.php: patch does not apply
error: patch failed: web/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php:131
error: web/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php: patch does not apply
error: web/core/tests/Drupal/KernelTests/Core/Http/BlockInterestCohortTest.php: No such file or directory
error: patch failed: web/sites/default/default.settings.php:593
error: web/sites/default/default.settings.php: patch does not apply
miedward’s picture

Version: 10.1.x-dev » 9.5.x-dev
miedward’s picture

Version: 9.5.x-dev » 10.1.x-dev

Misunderstood bot change.

idebr’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
6.4 KB
  1. Added a change record at https://www.drupal.org/node/3320787
  2. Added a deprecation \Drupal\Core\Site\Settings::$deprecatedSettings instructing developers to remove the block_interest_cohort Drupal setting.
  3. Added a deprecation assertion to \Drupal\Tests\Core\Site\SettingsTest::providerTestRealDeprecatedSettings
longwave’s picture

+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -37,7 +37,12 @@ final class Settings {
+      'message' => 'The "block_interest_cohort" setting is deprecated in drupal:9.5.0 and will be removed in drupal:9.5.0. This setting should be removed from the settings file. See https://www.drupal.org/node/3320787.',

Can we word this a bit differently? It's confusing to repeat the same version, although I know that is our standard format; we don't generally expect to deprecate and remove at the same time - though it makes sense here.

Status: Needs review » Needs work

The last submitted patch, 15: 3260401-15.patch, failed testing. View results

idebr’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
7.22 KB

#16 Reworded to: The "block_interest_cohort" setting is deprecated in drupal:9.5.0. This setting should be removed from the settings file, since its usage has been removed. See https://www.drupal.org/node/3320787.

longwave’s picture

Version: 10.1.x-dev » 9.5.x-dev
Status: Needs review » Needs work

This looks great to me for 10.x - but it doesn't apply to 9.5.x. Can you also roll a version for that, and we can hopefully get this in to the 9.5.0 release?

Akram Khan’s picture

trying to fix in 9.5.x address #19

Spokje’s picture

FileSize
7.44 KB
Akram Khan’s picture

fixing CCF Needs review

Spokje’s picture

Spokje’s picture

Status: Needs work » Needs review
longwave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this all looks good now.

#23 is for 9.5.x.
#18 is for 10.0.x/10.1.x.

longwave’s picture

Title: Google is abandoning FLoC - so remove the header? » Google is abandoning FLoC - so remove the header
catch’s picture

I went to try and commit this, and run into what I think is a completely unrelated phpstan issue:

----------------------------------------------------------------------------------------------------

Running PHPStan on changed files.
 ------ ------------------------------------------- 
  Line   sites/default/default.settings.php         
 ------ ------------------------------------------- 
  647    Variable $app_root might not be defined.   
  647    Variable $site_path might not be defined.  
 ------ ------------------------------------------- 


                                                                                
 [ERROR] Found 2 errors                                                         
 

Could probably commit it without running phpstan, but we will need an issue to sort that out either way. Might need a phpstan-ignore maybe.

Spokje’s picture

Hmm, when PHPStan is running on all files, per usual on Drupal CI, nothing is detected:

00:03:03.782 core/scripts/dev/commit-code-check.sh --drupalci

00:03:24.008 
1/4 ./assets/scaffold/files/default.settings.php 1049.17ms
00:03:25.346 
2/4 ./lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php 118.53ms
00:03:25.465 
3/4 ./lib/Drupal/Core/Site/Settings.php 87.24ms
00:03:25.554 
4/4 ./tests/Drupal/Tests/Core/Site/SettingsTest.php 89.67ms
00:03:25.644 CSpell: Files checked: 4, Issues found: 0 in 0 files
00:03:25.668 
00:03:25.668 CSpell: passed
00:03:25.668 
00:03:25.669 ----------------------------------------------------------------------------------------------------
00:03:25.669 
00:03:25.669 Running PHPStan on *all* files.

00:06:22.453 
00:06:22.459  [OK] No errors                                                                 
00:06:22.459 
00:06:22.487 
00:06:22.487 PHPStan: passed

Since DrupalCI can't replicate the problem, I can random add phpstan-ignore-next-line, but that seems a bit weird.

Maybe a core committer has to fix this themself?
And why are the differences between checking all and only changed files with PHPStan?

longwave’s picture

So these are in the baseline for the scaffold source version:

                -
                        message: "#^Variable \\$app_root might not be defined\\.$#"
                        count: 1
                        path: assets/scaffold/files/default.settings.php

                -
                        message: "#^Variable \\$site_path might not be defined\\.$#"
                        count: 1
                        path: assets/scaffold/files/default.settings.php

but not for the copy in sites/default/default.settings.php - but I can't immediately see why this is.

longwave’s picture

We already ignore settings.php, maybe we need to extend this to default.settings.php, given that it's an include and testing it standalone doesn't make sense?

    # Skip settings.
    - ../*/settings*.php
longwave’s picture

Oh, I see:

  paths:
    - .
    - ../composer

phpstan.neon.dist tells PHPStan to only analyse the core and composer top level directories by default, but not sites - however the precommit script doesn't take this into account, so if you explicitly specify a file outside of the listed paths, it will analyse it anyway; locally I get the same errors if I do this:

$ ../vendor/bin/phpstan analyze ../sites/default/default.settings.php
Note: Using configuration file /var/www/html/drupal/core/phpstan.neon.dist.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------- 
  Line   sites/default/default.settings.php         
 ------ ------------------------------------------- 
  662    Variable $app_root might not be defined.   
  662    Variable $site_path might not be defined.  
 ------ ------------------------------------------- 


                                                                                                                        
 [ERROR] Found 2 errors                                                                

Will open a followup to extend excludePaths to include default.settings.php.

Spokje’s picture

Ok, so do we add the errors to the baseline, or ignore the default.settings.php?

I like the latter more.
Added a patch to see if that would work.

longwave’s picture

Re #32 I think that is technically out of scope here so opened #3322182: Ignore sites directory in PHPStan

  • catch committed d9bdc53 on 10.0.x
    Issue #3260401 by idebr, Spokje, Akram Khan, mcdruid, longwave, alexpott...
  • catch committed af6cdbf on 10.1.x
    Issue #3260401 by idebr, Spokje, Akram Khan, mcdruid, longwave, alexpott...
  • catch committed 14b3f0b on 9.5.x
    Issue #3260401 by idebr, Spokje, Akram Khan, mcdruid, longwave, alexpott...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Since the phpstan issue is unrelated, I've gone ahead and committed this to 10.1.x/10.0.x/9.5.x with --no-verify to skip the phpstan check on commit.

idebr’s picture

Published the change record at https://www.drupal.org/node/3320787

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.