Problem/Motivation

Even at level 1, the phpstan job is failing for IEF.
https://git.drupalcode.org/issue/inline_entity_form-3410055/-/jobs/588375

 ------ ------------------------------------------------------------------- 
  Line   src/Element/InlineEntityForm.php                                   
 ------ ------------------------------------------------------------------- 
  200    Variable $inline_form_handler in empty() always exists and is not  
         falsy.                                                             
 ------ ------------------------------------------------------------------- 
 ------ ----------------------------------------------------------------------- 
  Line   src/Form/EntityInlineForm.php                                          
 ------ ----------------------------------------------------------------------- 
  282    Method Drupal\inline_entity_form\Form\EntityInlineForm::save() should  
         return int but return statement is missing.                            
 ------ ----------------------------------------------------------------------- 
 ------ ---------------------------------------------------------------------- 
  Line   src/MigrationHelper.php                                               
 ------ ---------------------------------------------------------------------- 
  29     \Drupal calls should be avoided in classes, use dependency injection  
         instead                                                               
  155    Call to static method load() on an unknown class                      
         Drupal\migrate_plus\Entity\MigrationGroup.                            
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols   
 ------ ---------------------------------------------------------------------- 
 ------ ---------------------------------------------------------------------- 
  Line   src/Plugin/Field/FieldWidget/InlineEntityFormBase.php                 
 ------ ---------------------------------------------------------------------- 
  471    \Drupal calls should be avoided in classes, use dependency injection  
         instead                                                               
  472    \Drupal calls should be avoided in classes, use dependency injection  
         instead                                                               
 ------ ---------------------------------------------------------------------- 
 ------ -------------------------------------------------------------------- 
  Line   tests/src/FunctionalJavascript/SimpleWidgetTest.php                 
 ------ -------------------------------------------------------------------- 
  177    Variable $host_node in isset() always exists and is not nullable.   
  184    Variable $child_node in isset() always exists and is not nullable.  
 ------ -------------------------------------------------------------------- 
 [ERROR] Found 8 errors

Steps to reproduce

Proposed resolution

  1. Fix the 8 errors above.
  2. Remove SKIP_PHPSTAN: 1 from .gitlab-ci.yml to get automated PHPStan tests passing again (at level 1).
  3. Open a follow-up to raise this to level 2 and fix those errors.
  4. Rinse + repeat.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork inline_entity_form-3413044

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

dww created an issue. See original summary.

dcam made their first commit to this issue’s fork.

dcam opened merge request !102

dcam’s picture

dcam commented
Status: Active » Needs review

All issues have been resolved except for two which I felt were best ignored for now.

nikolay shapovalov’s picture

nikolay shapovalov commented
Status: Needs review » Needs work
Issue tags: +PHPStan, +Dependency Injection (DI)

Thanks for the MR, I provide a feedback. Please check.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Related issues: +#3413233: Core requirements for 3.1.x: require D11.3+, PHP 8.3+

benjifisher made their first commit to this issue’s fork.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: Unassigned » benjifisher

I rebased and resolved merge conflicts caused by #3362087: Fix the issues reported by phpcs.

I made some changes to MigrationHelper in order to keep phpcs happy.

I am going to provide a BC layer, following Drupal deprecation policy > Constructor parameter additions.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Status: Needs work » Needs review

The MR is ready for review.

There are one or two open threads on the MR, but I think the intention is to open follow-up issues for them, As far as I am concerned, we do not need any more code changes for this issue.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: benjifisher » Unassigned
dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Almost there, thanks! Resolved a few MR threads, but opened a couple more.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review

Applied a few of my own suggestions 😅, responded to and resolved all threads.

I think this is RTBC now, but maybe I'll let someone else say so and I can then commit it. 😉

Cheers,
-Derek

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: Unassigned » benjifisher
Status: Needs review » Reviewed & tested by the community

@dww:

Congratulations on/thanks for joining as a co-maintainer of this module! I have a client who would like to see a stable release, so you can ping me in Slack if there is a task you would like me to do on the way there.

I reviewed the two commits that you added, and I updated the change record https://www.drupal.org/node/3417929 to match. LGTM

I am in the same boat as you: I have not stuck to the reviewer role on this issue. But if we are allowed to review each other's work, then I think we can (jointly) declare this issue to be RTBC.

dcam’s picture

dcam commented

I've reviewed the recent changes that have been made. They look good to me too. Thank you both for the help. RTBC+1.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: benjifisher » Unassigned

oops

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

In #3413233-8: Core requirements for 3.1.x: require D11.3+, PHP 8.3+, I pointed out that there is another constructor update (from #3224955: Allow themes to alter inline entity forms) that should have a BC layer. Since we already handle something similar for the MigrationHelper class in this issue, we might want to fix that here.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@benjifisher still plans to fix it here? (Otherwise let's maybe merge this, before it conflicts with many other MRs? You RTBC'd this 2024-01)

bluegeek9 made their first commit to this issue’s fork.

bluegeek9 changed the visibility of the branch 3.x to hidden.

bluegeek9’s picture

bluegeek9 commented

The remaining phpstan issue is resolved here: #3566408: Class Drupal\inline_entity_form\Element\InlineEntityForm extends deprecated class Drupal\Core\Render\Element\RenderElement.

  • dww committed cd5cf68f on 3.x 
    task: #3413044 Get PHPStan level 1 passing and enable automated test job...

  • dww committed 4466ce7f on 8.x-1.x 
    task: #3413044 Get PHPStan level 1 passing and enable automated test job...

dww closed merge request !102

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thank you very very much for solving this finally @dww and @bluegeek9!

@dww could you maybe have a look why the dev version is not shown for 3.x and there's no CI indicator on the module page?
https://www.drupal.org/project/inline_entity_form

Is 3.x disabled somehow?

Status: Fixed » Closed (fixed)

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