Problem/Motivation

Drupal 10 was recently released. Let's add explicit support for it in this module.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#17 interdiff-13_16.txt2.43 KBtim-diels
#17 3329686-16.patch5.79 KBtim-diels
#18 interdiff-13_18.txt2.43 KBtim-diels
#13 3329686-drupal-10-compatibility-2.patch3.75 KBprabuela
#18 interdiff-16_18.txt630 bytestim-diels
#10 3329686-drupal-10-compatibility-1.patch3.75 KBprabuela
#6 3329686-drupal-10-compatibility.patch3.75 KBprabuela
#18 3329686-18.patch5.78 KBtim-diels

Issue fork phone_number-3329686

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented 
8 warnings found.

web/modules/contrib/phone_number/modules/sms_phone_number/src/Plugin/Field/Field
Type/SmsPhoneNumberItem.php:
┌──────────┬──────┬──────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                            │
├──────────┼──────┼──────────────────────────────────────────────────────────────┤
│ Check    │ 324  │ Relying on entity queries to check access by default is      │
│ manually │      │ deprecated in drupal:9.2.0 and an error will be thrown from  │
│          │      │ drupal:10.0.0. Call                                          │
│          │      │ \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with │
│          │      │ TRUE or FALSE to specify whether access should be checked.   │
│          │      │                                                              │
│ Check    │ 409  │ Relying on entity queries to check access by default is      │
│ manually │      │ deprecated in drupal:9.2.0 and an error will be thrown from  │
│          │      │ drupal:10.0.0. Call                                          │
│          │      │ \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with │
│          │      │ TRUE or FALSE to specify whether access should be checked.   │
│          │      │                                                              │
│ Check    │ 416  │ Relying on entity queries to check access by default is      │
│ manually │      │ deprecated in drupal:9.2.0 and an error will be thrown from  │
│          │      │ drupal:10.0.0. Call                                          │
│          │      │ \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with │
│          │      │ TRUE or FALSE to specify whether access should be checked.   │
│          │      │                                                              │
└──────────┴──────┴──────────────────────────────────────────────────────────────┘

web/modules/contrib/phone_number/src/Plugin/Field/FieldType/PhoneNumberItem.php:
┌──────────┬──────┬──────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                            │
├──────────┼──────┼──────────────────────────────────────────────────────────────┤
│ Check    │ 352  │ Relying on entity queries to check access by default is      │
│ manually │      │ deprecated in drupal:9.2.0 and an error will be thrown from  │
│          │      │ drupal:10.0.0. Call                                          │
│          │      │ \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with │
│          │      │ TRUE or FALSE to specify whether access should be checked.   │
│          │      │                                                              │
└──────────┴──────┴──────────────────────────────────────────────────────────────┘

web/modules/contrib/phone_number/phone_number.libraries.yml:
┌──────────┬──────┬─────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                           │
├──────────┼──────┼─────────────────────────────────────────────────────────────┤
│ Check    │ 0    │ The 'element' library is depending on a deprecated library. │
│ manually │      │ The core/jquery.once asset library is deprecated in Drupal  │
│          │      │ 9.3.0 and will be removed in Drupal 10.0.0. Use the         │
│          │      │ core/once library instead. See                              │
│          │      │ https://www.drupal.org/node/3158256                         │
│          │      │                                                             │
└──────────┴──────┴─────────────────────────────────────────────────────────────┘

web/modules/contrib/phone_number/modules/sms_phone_number/src/Element/SmsPhoneNu
mber.php:
┌──────────┬──────┬─────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                         MESSAGE                         │
├──────────┼──────┼─────────────────────────────────────────────────────────┤
│ Check    │ 104  │ The 'sms_phone_number/element' library is not defined   │
│ manually │      │ because the defining extension is not installed. Cannot │
│          │      │ decide if it is deprecated or not.                      │
│          │      │                                                         │
└──────────┴──────┴─────────────────────────────────────────────────────────┘

web/modules/contrib/phone_number/phone_number.info.yml:
┌──────────┬──────┬────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                          MESSAGE                           │
├──────────┼──────┼────────────────────────────────────────────────────────────┤
│ Check    │ 0    │ Value of core_version_requirement: ^8 || ^9 is not         │
│ manually │      │ compatible with the next major version of Drupal core. See │
│          │      │ https://drupal.org/node/3070687.                           │
│          │      │                                                            │
└──────────┴──────┴────────────────────────────────────────────────────────────┘

web/modules/contrib/phone_number/modules/sms_phone_number/sms_phone_number.info.
yml:
┌──────────┬──────┬────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                          MESSAGE                           │
├──────────┼──────┼────────────────────────────────────────────────────────────┤
│ Check    │ 0    │ Value of core_version_requirement: ^8 || ^9 is not         │
│ manually │      │ compatible with the next major version of Drupal core. See │
│          │      │ https://drupal.org/node/3070687.                           │
│          │      │                                                            │
└──────────┴──────┴────────────────────────────────────────────────────────────┘
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Issue tags: +Drupal 10 compatibility
prabuela’s picture

prabuela commented
Assigned: Unassigned » prabuela
prabuela’s picture

prabuela commented
Assigned: prabuela » Unassigned
prabuela’s picture

prabuela commented
Assigned: Unassigned » prabuela
StatusFileSize
new 3329686-drupal-10-compatibility.patch3.75 KB
prabuela’s picture

prabuela commented
Status: Active » Needs review
prabuela’s picture

prabuela commented
Assigned: prabuela » Unassigned
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work 
  1. +++ b/modules/sms_phone_number/sms_phone_number.info.yml
@@ -2,7 +2,7 @@ name: SMS Phone Number
+core_version_requirement: ^9 || ^10

+++ b/phone_number.info.yml
@@ -2,7 +2,7 @@ name: Phone Number
+core_version_requirement: ^9 || ^10

    once js library was only added in 9.2, so this should change to 9.2 || ^10 

  2. +++ b/modules/sms_phone_number/src/Plugin/Field/FieldType/SmsPhoneNumberItem.php
@@ -323,6 +323,7 @@ public function isVerified() {
+      ->accessCheck(FALSE)

@@ -399,6 +400,7 @@ public function isUnique($unique_type = SmsPhoneNumberUtilInterface::PHONE_NUMBE
+      ->accessCheck(FALSE)

+++ b/src/Plugin/Field/FieldType/PhoneNumberItem.php
@@ -345,6 +345,7 @@ public function isUnique() {
+      ->accessCheck(FALSE)

    The default without adding accessCheck is TRUE. Is there a reason to change from the default of TRUE to a FALSE? I'd suggest this should change back to TRUE.

prabuela’s picture

prabuela commented
StatusFileSize
new 3329686-drupal-10-compatibility-1.patch3.75 KB

1. 9.2 is updated.

2. Here is the explanation for True and False.

// Unchanged: This gets all articles the current user can view.
$ids = \Drupal::entityQuery('node')
->accessCheck(TRUE)
 ->condition('type', 'article')
->execute();

// Unchanged: This gets all articles that exist regardless of access.
$ids = \Drupal::entityQuery('node')
->accessCheck(FALSE)
 ->condition('type', 'article')
->execute();

prabuela’s picture

prabuela commented
Status: Needs work » Needs review
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Yes, but adding ->accessCheck(FALSE) is a change of functionality for the module. If no ->accessCheck() is added, the default was TRUE. If someone had some custom field access setup for a phone field, that would be a change in functionality for that site. Right? Also, there looks to be 3 accessCheck additions needed in SmsPhoneNumberItem, not just 2.

prabuela’s picture

prabuela commented
StatusFileSize
new 3329686-drupal-10-compatibility-2.patch3.75 KB

Yes, You are correct whereas, if we want to display only for the current user we can add
(->accessCheck(TRUE)) attached patch below.

For Example:
// This gets all articles the current user can view.
$ids = \Drupal::entityQuery('node')
->condition('type', 'article')
->execute();

// This also gets all articles the current user can view.
$ids = \Drupal::entityQuery('node')
->accessCheck(TRUE)
->condition('type', 'article')
->execute();

There are two entity query is used in the file SmsPhoneNumberItem.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Reviewed & tested by the community

Looking good.

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
Status: Reviewed & tested by the community » Needs work

Thanks already for the work done!

But the sub module sms_phone_number should also be updated regarding the once library.

And after manually testing I get the error: Uncaught TypeError: once(...).each is not a function and the flags are not shown.

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
Status: Needs work » Reviewed & tested by the community

I've updated the patch to also include the JS in the submodule.

Upgrade status is green.
Manual testings seems fine.

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
StatusFileSize
new 3329686-16.patch5.79 KB
new interdiff-13_16.txt2.43 KB
tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
StatusFileSize
new 3329686-18.patch5.78 KB
new interdiff-16_18.txt630 bytes
new interdiff-13_18.txt2.43 KB

To fast. Updated patch.

  • tim-diels committed 1f15a374 on 8.x-1.x 
    Issue #3329686 by PrabuEla, tim-diels, heddn: Drupal 10 compatibility
tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
Status: Reviewed & tested by the community » Fixed

Thanks everyone!

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Fixed » Needs work

On really small nit. And maybe it doesn't matter. And this is looking really good. Thanks for catching the sms omissions. We weren't using it so I forgot to check it.

--- a/js/phone-number-form-element.js
+++ b/js/phone-number-form-element.js

--- a/modules/sms_phone_number/js/sms-phone-number-form-element.js
+++ b/modules/sms_phone_number/js/sms-phone-number-form-element.js

This is missing an update to the sms.libraries.yml to include core/once.

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented
Status: Needs work » Fixed

As sms_phone_number/element has a dependency on phone_number/element, the dependency should not be needed there?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

It is a minor thing. But since sms uses once directly and (maybe) someday element might stop using it, it would be safer to include all direct dependencies in sms. But not a huge issue either way. It is a small nit. Thanks for taking this over the finish line.

tim-diels’s picture

tim-diels
He/Him
Primary language Dutch
Location Belgium 🇧🇪
commented

I'll create a follow up issue to address this and fix it there. Thank you also for setting this up to get things going!

Status: Fixed » Closed (fixed)

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