Drupal 9 is coming, lets make this module ready!

CommentFileSizeAuthor
#17 interdiff-3113356-14-17.txt438 bytesmrinalini9
#17 3113356-17.patch19.72 KBmrinalini9
#14 interdiff_12-14.txt1.66 KBmitrpaka
#14 3113356-14.patch19.12 KBmitrpaka
#12 interdiff_7-12.txt4.7 KBridhimaabrol24
#12 3113356-12.patch19.12 KBridhimaabrol24
#7 3113356-7.patch20.31 KBfastangel
#6 interdiff_4-6.txt13.58 KBmitrpaka
#6 interdiff_5-6.txt10.6 KBmitrpaka
#6 3113356-6.patch20.07 KBmitrpaka
#5 interdiff_4-5.txt5.31 KBmitrpaka
#5 3113356-5.patch11.42 KBmitrpaka
#4 3113356-4.patch5.89 KBsahana _n
#2 3113356.patch463 byteseiriksm

Comments

eiriksm created an issue. See original summary.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Active » Needs review
StatusFileSize
new 3113356.patch463 bytes
imiksu’s picture

imiksu
Location Finland
commented
Status: Needs review » Needs work

According to my drupal-check run I found following errors:

 ------ ----------------------------------------------------- 
  Line   elasticsearch_helper/elasticsearch_helper.drush.inc  
 ------ ----------------------------------------------------- 
  110    Call to deprecated function drush_print().           
  117    Call to deprecated function drush_print().           
 ------ ----------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------ 
  Line   elasticsearch_helper/modules/elasticsearch_helper_aws/src/Form/AwsSettingsForm.php        
 ------ ------------------------------------------------------------------------------------------ 
         Class Drupal\elasticsearch_helper_aws\Form\AWSSettingsForm was not found while trying to  
         analyse it - autoloading is probably not configured properly.                             
 ------ ------------------------------------------------------------------------------------------ 

 ------ -------------------------------------------------------------------------------------------------------------------- 
  Line   elasticsearch_helper/modules/elasticsearch_helper_content/src/Plugin/Normalizer/ElasticsearchContentNormalizer.php  
 ------ -------------------------------------------------------------------------------------------------------------------- 
  281    Call to deprecated function entity_get_display():                                                                   
         in drupal:8.8.0 and is removed from drupal:9.0.0. Use                                                               
         EntityDisplayRepositoryInterface::getViewDisplay() instead.                                                         
 ------ -------------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------- 
  Line   elasticsearch_helper/modules/elasticsearch_helper_views/src/Plugin/views/field/RenderedEntity.php  
 ------ --------------------------------------------------------------------------------------------------- 
  54     Parameter $entity_manager of method                                                                
         Drupal\elasticsearch_helper_views\Plugin\views\field\RenderedEntity::__construct() has             
         typehint with deprecated interface Drupal\Core\Entity\EntityManagerInterface:                      
         in drupal:8.0.0 and is removed from drupal:9.0.0.                                                  
 ------ --------------------------------------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------- 
  Line   elasticsearch_helper/src/Form/ElasticsearchHelperSettingsForm.php     
 ------ ---------------------------------------------------------------------- 
  75     Call to deprecated function drupal_set_message():                     
         in drupal:8.5.0 and is removed from drupal:9.0.0.                     
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.  
  83     Call to deprecated function drupal_set_message():                     
         in drupal:8.5.0 and is removed from drupal:9.0.0.                     
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.  
  88     Call to deprecated function drupal_set_message():                     
         in drupal:8.5.0 and is removed from drupal:9.0.0.                     
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.  
  91     Call to deprecated function drupal_set_message():                     
         in drupal:8.5.0 and is removed from drupal:9.0.0.                     
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.  
 ------ ---------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------- 
  Line   elasticsearch_helper/src/Plugin/ElasticsearchIndexBase.php            
 ------ ---------------------------------------------------------------------- 
  122    Call to deprecated function drupal_set_message():                     
         in drupal:8.5.0 and is removed from drupal:9.0.0.                     
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.  
  130    Call to deprecated function drupal_set_message():                     
         in drupal:8.5.0 and is removed from drupal:9.0.0.                     
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.  
 ------ ---------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------ 
  Line   elasticsearch_helper/tests/src/FunctionalJavascript/EntityOpsTest.php               
 ------ ------------------------------------------------------------------------------------ 
  13     Class Drupal\Tests\elasticsearch_helper\FunctionalJavascript\EntityOpsTest extends  
         deprecated class Drupal\FunctionalJavascriptTests\JavascriptTestBase:               
         in drupal:8.6.0 and is removed from drupal:9.0.0.                                   
         Use \Drupal\FunctionalJavascriptTests\WebDriverTestBase instead                     
 ------ ------------------------------------------------------------------------------------
sahana _n’s picture

sahana _n commented
Status: Needs work » Needs review
StatusFileSize
new 3113356-4.patch5.89 KB

Please review the patch.

mitrpaka’s picture

mitrpaka commented
StatusFileSize
new 3113356-5.patch11.42 KB
new interdiff_4-5.txt5.31 KB

Thanks for the patches. Here is updated patch with some additions to previous ones.

Checked with https://www.drupal.org/project/upgrade_status - No known issues found.

mitrpaka’s picture

mitrpaka commented
StatusFileSize
new 3113356-6.patch20.07 KB
new interdiff_5-6.txt10.6 KB
new interdiff_4-6.txt13.58 KB

Updated patch.

fastangel’s picture

fastangel commented
StatusFileSize
new 3113356-7.patch20.31 KB

I tried the last patch and is getting problem with drupal 9 install with:
Drupal\Core\Extension\InfoParserException: The 'core_version_requirement' can not be used to specify compatibility for a specific [error]
version before 8.7.7

I attach the patch with the fix.

jcnventura’s picture

jcnventura
Primary language English
commented
Issue tags: +Drupal 9 porting weekend, +Drupal 9 compatibility
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Needs work 
  1. +++ b/elasticsearch_helper.info.yml
@@ -1,8 +1,9 @@
+  - drupal:system (>=8.7)

+++ b/modules/elasticsearch_helper_aws/elasticsearch_helper_aws.info.yml
@@ -1,8 +1,9 @@
+- drupal:system (>=8.7)

+++ b/modules/elasticsearch_helper_index_alias/elasticsearch_helper_index_alias.info.yml
@@ -1,7 +1,8 @@
+  - drupal:system (>=8.7)

+++ b/modules/elasticsearch_helper_views/elasticsearch_helper_views.info.yml
@@ -1,7 +1,8 @@
+  - drupal:system (>=8.7)

    This isn't necessary to add to the info.yml file if the core_version_requirement is also ^8.7.7 || ^9. Removing core: 8.x and adding core_version_requirement: ^8.7.7 || ^9 is enough to make the requirement on Drupal core. The system module is a mandatory core module that is always installed. 

  2. +++ b/modules/elasticsearch_helper_content/elasticsearch_helper_content.info.yml
@@ -1,7 +1,7 @@
+core_version_requirement: ^8.8.0 || ^9

+++ b/modules/elasticsearch_helper_instant/elasticsearch_helper_instant.info.yml
@@ -1,8 +1,9 @@
+core_version_requirement: ^8.8.0 || ^9

    This seems odd to me, but I guess if that's the pattern that's being used to separate versions in this contrib module that's okay.

    However it should be core_version_requirement: ^8.8 || ^9 without the patch version as documented on specifying the core_version_requirement when needed. 

  3. +++ b/modules/elasticsearch_helper_views/src/Plugin/views/field/RenderedEntity.php
@@ -302,9 +304,18 @@ class RenderedEntity extends FieldPluginBase implements CacheableDependencyInter
     return $this->entityManager;

    If this is ever called, there will be an error thrown because the entityManager property doesn't exist? Unless it exists on the base class?

    If we want to use this pattern then we should add a protected $deprecatedProperties array similar to the core class of the same name.

    Otherwise this will throw an exception because $deprecatedProperties property doesn't exist on this class.

jcnventura’s picture

jcnventura
Primary language English
commented

Also, makes no sense to specify 8.7.7, as that is the minimum version that support core_version_requirement. Might be better to just state ^8 || ^9. However, seeing that part of the module requires 8.8.0, and as we can't have multiple composer.json files, I think this module must declare the following in all the .info.yml files: ^8.8 || ^9

ridhimaabrol24’s picture

ridhimaabrol24 commented
Assigned: Unassigned » ridhimaabrol24
ridhimaabrol24’s picture

ridhimaabrol24 commented
Assigned: ridhimaabrol24 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3113356-12.patch19.12 KB
new interdiff_7-12.txt4.7 KB

Patch #7 failed to apply on the latest branch. Fixed the issues and implemented suggestion from #9 and #10. Providing interdiff as well for clarity.

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Needs work

ALL modules must have core_version_requirement except the one belonging to the 'Testing' package.

mitrpaka’s picture

mitrpaka commented
Status: Needs work » Needs review
StatusFileSize
new 3113356-14.patch19.12 KB
new interdiff_12-14.txt1.66 KB

Maybe we should add note to README.md that prior to 8.8 version there is need to use earlier dev version of the module as entity_get_display() was moved to EntityDisplayRepositoryInterface in 8.8.x (https://www.drupal.org/node/2835616)

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Needs work

There's still a use of @entity.manager in elasticsearch_helper/examples/elasticsearch_helper_example/elasticsearch_helper_example.services.yml

mrinalini9’s picture

mrinalini9 commented
Assigned: Unassigned » mrinalini9
mrinalini9’s picture

mrinalini9 commented
Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3113356-17.patch19.72 KB
new interdiff-3113356-14-17.txt438 bytes

Updated patch as per the changes mentioned in #15, please review.

jcnventura credited maijs.

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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