Problem/Motivation

\Drupal\Core\Field\FieldStorageDefinitionInterface::isQueryable() was originally introduced in #1696640-57: Implement API to unify entity properties and fields and moved around quite a bit since then, but it was never used and its purpose has since been replaced by \Drupal\Core\Field\FieldStorageDefinitionInterface::hasCustomStorage() or \Drupal\Core\TypedData\DataDefinitionInterface::isComputed()

Proposed resolution

Deprecate \Drupal\Core\Field\FieldStorageDefinitionInterface::isQueryable() and remove \Drupal\Core\Field\BaseFieldDefinition::setQueryable().

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#12 interdiff-2855886-8-12.txt1.32 KBhimanshu-dixit
#12 2855886-12.patch3.57 KBhimanshu-dixit
#8 interdiff-2855886-5-8.txt1.79 KBshadcn
#8 deprecate-2855886-8.patch3.57 KBshadcn
#5 interdiff-2855886-2-5.txt1.32 KBshadcn
#5 deprecate-2855886-5.patch2.91 KBshadcn
#2 2855886.patch2.85 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2855886.patch2.85 KB

Something like this.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

What if a contrib module is using setQueryable()? can we just remove it?

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

That's a good question.. we should probably deprecate instead of removing it.

shadcn’s picture

shadcn CreditAttribution: shadcn at Chapter Three commented
FileSize
deprecate-2855886-5.patch2.91 KB
interdiff-2855886-2-5.txt1.32 KB

Deprecated as per #4.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

@amateescu, it'd be good if you could +1 #5 too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Yep this does not totally extraneous. We're in the process of putting together a policy on how to deprecate things. The basic bit is that we should add an @trigger_error.

One of the things that is important is the trigger_error links to the change record for the deprecation - which this issue is missing.

Here's the current text for deprecating methods:

Add @trigger_error('...', E_USER_DEPRECATED) at the top of the method. Add @deprecated to the docblock for the method. For example:

/**
 * Checks if a string is safe to output.
 *
 * @param string|\Drupal\Component\Render\MarkupInterface $string
 *   The content to be checked.
 * @param string $strategy
 *   (optional) This value is ignored.
 *
 * @return bool
 *   TRUE if the string has been marked secure, FALSE otherwise.
 *
 * @deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0.
 *   Instead, you should just check if a variable is an instance of
 *   \Drupal\Component\Render\MarkupInterface.
 *
 * @see https://www.drupal.org/node/2549395
 */
public static function isSafe($string, $strategy = 'html') {
  @trigger_error('SafeMarkup::isSafe() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Instead, you should just check if a variable is an instance of \Drupal\Component\Render\MarkupInterface. See https://www.drupal.org/node/2549395.', E_USER_DEPRECATED);
  return $string instanceof MarkupInterface;
}
shadcn’s picture

shadcn CreditAttribution: shadcn at Chapter Three commented
Status: Needs work » Needs review
FileSize
deprecate-2855886-8.patch3.57 KB
interdiff-2855886-5-8.txt1.79 KB

Thanks @alexpott. Here's a first draft: https://www.drupal.org/node/2856563

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Thanks @arshadcn, back to RTBC.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 
+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -287,7 +286,8 @@ public function isMultiple() {
+    @trigger_error('BaseFieldDefinition::isQueryable() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal\Core\Field\BaseFieldDefinition::hasCustomStorage(). See https://www.drupal.org/node/2856563.', E_USER_DEPRECATED);

@@ -298,8 +298,14 @@ public function isQueryable() {
+   * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use
...
+    @trigger_error('BaseFieldDefinition::setQueryable() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal\Core\Field\BaseFieldDefinition::setCustomStorage(). See https://www.drupal.org/node/2856563.', E_USER_DEPRECATED);

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
@@ -109,6 +109,12 @@ public function isRevisionable();
+   * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use

Shouldn't we have the same message here? 8.4.0 vs. 8.4.x and the same for 9.0.0 and 9.0.x :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work

Yep, they should end in .0 for clarity. 8.4.0 is a specific tag/tarball on a specific date. 8.4.x is a year's worth of ever-changing code in development and then stable. :)

For reference, the policy is live here now: https://www.drupal.org/core/deprecation

himanshu-dixit’s picture

himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commented
Status: Needs work » Needs review
FileSize
2855886-12.patch3.57 KB
interdiff-2855886-8-12.txt1.32 KB

Here is the new patch which updates the documentation

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

  • catch committed 5071f6d on 8.4.x 
    Issue #2855886 by arshadcn, himanshu-dixit, amateescu: Deprecate \Drupal...
catch’s picture

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

Committed/pushed to 8.4.x, thanks!

Status: Fixed » Closed (fixed)

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

himanshu-dixit’s picture

himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer and at Google Summer of Code commented

Attributing the contribution to GSoC.