When visiting http://example.com/admin/config/search/search-api/index/sandbox_node_ind...

Strict warning: Drupal\Core\Entity\EntityForm and Drupal\search_api\Form\UnsavedConfigurationFormTrait define the same property ($entityTypeManager) in the composition of Drupal\search_api\Form\IndexFieldsForm. This might be incompatible, to improve maintainability consider using accessor methods in traits instead. Class was composed in include() (line 433 of modules/contrib/search_api/src/Form/IndexFieldsForm.php).

CommentFileSizeAuthor
#8 2803701-8--php5_strict_warning.patch1.21 KBdrunken monkey
#5 extra_entity_type_manager_prop-2803701-5.patch3.53 KBrbayliss
#3 extra_entity_type_manager_prop-2803701-3.patch2.63 KBrbayliss

Comments

Boobaa created an issue. See original summary.

sebas5384’s picture

sebas5384 commented

I can confirm this warning is happening.

rbayliss’s picture

rbayliss commented
Status: Active » Needs review
StatusFileSize
new extra_entity_type_manager_prop-2803701-3.patch2.63 KB

Yeah, I can confirm. Here's a stab at fixing it.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work 
+++ b/src/Form/IndexAddFieldsForm.php
@@ -290,7 +290,7 @@ class IndexAddFieldsForm extends EntityForm {
-          $entity_type = $this->getEntityTypeManager()
+          $entity_type = $this->entityTypeManager

+++ b/src/Form/IndexFieldsForm.php
@@ -400,7 +400,7 @@ class IndexFieldsForm extends EntityForm {
-      $index->savePermanent($this->getEntityTypeManager());
+      $index->savePermanent($this->entityTypeManager);

+++ b/src/Form/UnsavedConfigurationFormTrait.php
@@ -83,7 +66,7 @@ trait UnsavedConfigurationFormTrait {
-          '#account' => $entity->getLockOwner($this->entityTypeManager),
+          '#account' => $entity->getLockOwner(),

Not sure this change is needed.

rbayliss’s picture

rbayliss commented
Status: Needs work » Needs review
StatusFileSize
new extra_entity_type_manager_prop-2803701-5.patch3.53 KB

I think you're just talking about the last one, right? (not explicitly passing in $this->entityTypeManager in the trait). The reason this change was made is that there is no longer an entityTypeManager on the trait - it belongs to EntityForm, and the trait doesn't have any knowledge of it since you could easily implement this trait on a non-EntityForm class. If we need to be explicit there, we should add an abstract getEntityTypeManager() to the trait and implement it on the classes that use the trait. Rerolling with this change.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

@rbayliss, not sure why you made an abstract function on the trait and you didn't just put the method in the trait? Anyway - not too fussed about that and it does resolve the issue. So setting to RTBC.

tregismoreira’s picture

tregismoreira commented

The patch #5 worked for me. Thanks! ;)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2803701-8--php5_strict_warning.patch1.21 KB

Ah, yes, that's a pretty stupid problem in PHP 5. It's fixed in PHP 7, but that unfortunately doesn't help us much for the module. We've run into it before. Thanks for reporting it!

The proposed solution is way too complicated, though – why not just this? (I would even argue the isset() check isn't needed, but it's not much of a problem either way.)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Yeah, that does look a lot simpler!

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Related issues: +#2817999: Strict warnings

This patch reduces the amount of fails at https://travis-ci.org/mkalkbrenner/search_api_integration_tests/builds/1... a lot compared to https://travis-ci.org/mkalkbrenner/search_api_integration_tests/builds/1...

Can we commit it soon?

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

without patch:

rupal\core_search_facets\Tests\HooksTest                     19 passes                            5 messages
Drupal\rest_facets\Tests\RestIntegrationTest                  73 passes                           16 messages
Drupal\facets\Tests\FacetSourceTest                           64 passes                           12 messages
Drupal\core_search_facets\Tests\IntegrationTest              222 passes   8 fails                 64 messages
Drupal\facets\Tests\UrlIntegrationTest                       113 passes                           24 messages
Drupal\facets\Tests\LanguageIntegrationTest                   32 passes                            7 messages
Drupal\facets\Tests\ProcessorIntegrationTest                 432 passes                          120 messages
Drupal\facets\Tests\WidgetIntegrationTest                    148 passes                           42 messages
Drupal\search_api_solr_defaults\Tests\IntegrationTest         99 passes                           33 messages
Drupal\search_api_solr\Tests\IntegrationTest                 265 passes                           76 messages
Drupal\search_api_db\Tests\IntegrationTest                    15 passes                            4 messages
Drupal\search_api\Tests\LocalActionsWebTest                   15 passes                            3 messages
Drupal\search_api_db_defaults\Tests\IntegrationTest          135 passes             3 exceptions  47 messages
Drupal\search_api_solr\Tests\ViewsTest                       362 passes                           75 messages
Drupal\search_api\Tests\ProcessorIntegrationTest             159 passes            10 exceptions  35 messages
Drupal\search_api\Tests\CacheabilityTest                      21 passes                            3 messages
Drupal\search_api\Tests\LanguageIntegrationTest               70 passes                           25 messages
Drupal\search_api\Tests\HooksTest                             59 passes             2 exceptions  15 messages
Drupal\search_api\Tests\IntegrationTest                      932 passes   6 fails 107 exceptions 263 messages
Drupal\search_api\Tests\OverviewPageTest                     112 passes                           30 messages
Drupal\search_api\Tests\ViewsTest                            358 passes                           74 messages
Drupal\search_api_solr_multilingual\Tests\IntegrationTest    265 passes                           76 messages
Drupal\search_api_solr_multilingual\Tests\ViewsTest          362 passes                           75 messages
Drupal\facets\Tests\IntegrationTest                          824 passes   4 fails                223 messages

with patch applied, all exceptions are gone:

Drupal\core_search_facets\Tests\HooksTest                     19 passes                                      
Drupal\rest_facets\Tests\RestIntegrationTest                  73 passes                                      
Drupal\facets\Tests\FacetSourceTest                           64 passes                                      
Drupal\core_search_facets\Tests\IntegrationTest              222 passes   8 fails                            
Drupal\facets\Tests\UrlIntegrationTest                       113 passes                                      
Drupal\facets\Tests\LanguageIntegrationTest                   32 passes                                      
Drupal\facets\Tests\ProcessorIntegrationTest                 432 passes                                      
Drupal\facets\Tests\WidgetIntegrationTest                    148 passes                                      
Drupal\search_api_solr_defaults\Tests\IntegrationTest         99 passes                                      
Drupal\search_api_solr\Tests\IntegrationTest                 265 passes                                      
Drupal\search_api_db\Tests\IntegrationTest                    15 passes                                      
Drupal\search_api\Tests\LocalActionsWebTest                   15 passes                                      
Drupal\search_api_db_defaults\Tests\IntegrationTest          135 passes                                      
Drupal\search_api_solr\Tests\ViewsTest                       362 passes                                      
Drupal\search_api\Tests\ProcessorIntegrationTest             159 passes                                      
Drupal\search_api\Tests\CacheabilityTest                      21 passes                                      
Drupal\search_api\Tests\LanguageIntegrationTest               70 passes                                      
Drupal\search_api\Tests\HooksTest                             59 passes                                      
Drupal\search_api\Tests\OverviewPageTest                     112 passes                                      
Drupal\search_api\Tests\ViewsTest                            358 passes                                      
Drupal\search_api\Tests\IntegrationTest                      932 passes   6 fails                            
Drupal\search_api_solr_multilingual\Tests\IntegrationTest    265 passes                                      
Drupal\search_api_solr_multilingual\Tests\ViewsTest          362 passes                                      
Drupal\facets\Tests\IntegrationTest                          824 passes   4 fails
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Good to hear, thanks for reviewing!
Committed.
Thanks again, everyone!

  • drunken monkey committed 1e4de5c on 8.x-1.x 
    Issue #2803701 by drunken monkey, rbayliss: Fixed strict warnings from...

Status: Fixed » Closed (fixed)

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