Problem/Motivation

We are currently skipping the Install profile will be a mandatory parameter in Drupal 9.0. deprecation.

Proposed resolution

Remove deprecation skipping and fix how it is triggered to be only when \Drupal::installProfile() results in setting a profile value.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#19 2959282-2-19.patch5.44 KBalexpott
#19 16-19-interdiff.txt2.06 KBalexpott
#16 2959282-2-16.patch5.42 KBalexpott
#16 15-16-interdiff.txt4.76 KBalexpott
#15 2959282-15.patch3.98 KBLendude
#15 interdiff-2959282-11-15.txt3.35 KBLendude
#11 2959282_11.patch3.9 KBMile23
#7 2959282-7.patch3.74 KBalexpott
#7 6-7-interdiff.txt1.86 KBalexpott
#6 2959282-6.patch1.88 KBalexpott
#4 2959282-4.patch916 bytesjibran
#3 2959282-3.patch916 bytesjibran
#2 2959282-2.patch2.2 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Active » Needs review
FileSize
2959282-2.patch2.2 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
2959282-3.patch916 bytes

Let's simplify the logic.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
2959282-4.patch916 bytes

Correct patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Needs review » Needs work

It's very debatable if assignment in an if with other conditions is simplying anything.

Also the latest patch does not remove the deprecation message for getSkippedDeprecations().

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Needs work » Needs review
FileSize
2959282-6.patch1.88 KB

Re-uploading #2 after re-rolling.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
FileSize
6-7-interdiff.txt1.86 KB
2959282-7.patch3.74 KB

Added a test because we have logic about when and how the deprecation is triggered.

Status: Needs review » Needs work

The last submitted patch, 7: 2959282-7.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Needs work » Needs review

#8 Unrelated test fail in Drupal\Tests\quickedit\FunctionalJavascript\FieldTest

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Parent issue: » #2959269: [meta] Core should not trigger deprecated code except in tests and during updates
FileSize
2959282_11.patch3.9 KB

Reroll.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue tags: +Needs change record updates

It looks like this deprecation comes from #2156401: Write install_profile value to configuration and only to settings.php if it is writeable which also deprecates a lot of other top-level functions. None were committed with change record @see, but some have them now. None of them appear to have @trigger_error, either, though I'd assume there's a reason.

Anyway, poking around led me to this CR but I'm not sure it's the right one: https://www.drupal.org/node/2538996

Adding 'CR updates' because we need to point to a CR from the error message and the deprecation notice.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Needs work

Patch still applies. :-) I guess I should have said 'needs work' in october.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at ezCompany commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates
FileSize
interdiff-2959282-11-15.txt3.35 KB
2959282-15.patch3.98 KB

The change record in #12 seems pretty appropriate, so let's use that. Also updated the test so that all three scenarios are actually run.

edit: interdiff looks a bit weird for the getSkippedDeprecations change

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
FileSize
15-16-interdiff.txt4.76 KB
2959282-2-16.patch5.42 KB

After discussing this a bit with @berdir I realised that this is confusing because in Drupal 9 all of the constructor params are going to have to be required so really that's what we need to deprecate for.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer and at ICON Interactive commented
Status: Needs review » Reviewed & tested by the community

That makes more sense to me. Now you can still call the constructor with a NULL profile, you just have to explicitly call it with NULL. It's cleaner.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
@@ -41,26 +43,27 @@ class ExtensionInstallStorage extends InstallStorage {
+    if (count(func_get_args()) < 5) {

    Should this be !isset($profile) before the count()? I realise it's not a code path that needs micro-optimizing though.

  2. +++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
@@ -41,26 +43,27 @@ class ExtensionInstallStorage extends InstallStorage {
+      @trigger_error('All \Drupal\Core\Config\ExtensionInstallStorage::__construct() arguments will be required in Drupal 9.0. See https://www.drupal.org/node/2538996', E_USER_DEPRECATED);

    9.0.0?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Needs work » Reviewed & tested by the community
FileSize
16-19-interdiff.txt2.06 KB
2959282-2-19.patch5.44 KB

@catch thanks for the review.
1. Sure why not - can't hurt I guess.
2. Fixed.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 36dd174 and pushed to 8.8.x. Thanks!

  • catch committed 36dd174 on 8.8.x 
    Issue #2959282 by alexpott, jibran, Mile23: Fix "Install profile will be...

Status: Fixed » Closed (fixed)

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