Fix deprecated extending of render element

Created on 25 June 2024, 8 months ago
Updated 8 September 2024, 5 months ago

Problem/Motivation

These are causing test failures in any current issues with merge requests

php vendor/bin/phpstan analyze $_WEB_ROOT/modules/custom/$CI_PROJECT_NAME $PHPSTAN_CONFIGURATION --no-progress || EXIT_CODE=$?
 ------ -------------------------------------------------------------- 
  Line   modules/field_group_accordion/src/Element/Accordion.php       
 ------ -------------------------------------------------------------- 
  13     Class Drupal\field_group_accordion\Element\Accordion extends  
         deprecated class Drupal\Core\Render\Element\RenderElement:    
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use       
           \Drupal\Core\Render\Element\RenderElementBase instead.      
 ------ -------------------------------------------------------------- 
 ------ ------------------------------------------------------------------ 
  Line   modules/field_group_accordion/src/Element/AccordionItem.php       
 ------ ------------------------------------------------------------------ 
  12     Class Drupal\field_group_accordion\Element\AccordionItem extends  
         deprecated class Drupal\Core\Render\Element\RenderElement:        
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use           
           \Drupal\Core\Render\Element\RenderElementBase instead.          
 ------ ------------------------------------------------------------------ 
 ------ -------------------------------------------------------------------- 
  Line   src/Element/HorizontalTabs.php                                      
 ------ -------------------------------------------------------------------- 
  17     Class Drupal\field_group\Element\HorizontalTabs extends deprecated  
         class Drupal\Core\Render\Element\RenderElement:                     
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use             
           \Drupal\Core\Render\Element\RenderElementBase instead.            
 ------ -------------------------------------------------------------------- 
 ------ ----------------------------------------------------------------------- 
  Line   src/Element/HtmlElement.php                                            
 ------ ----------------------------------------------------------------------- 
  14     Class Drupal\field_group\Element\HtmlElement extends deprecated class  
         Drupal\Core\Render\Element\RenderElement:                              
         in drupal:10.3.0 and is removed from drupal:12.0.0. Use                
           \Drupal\Core\Render\Element\RenderElementBase instead.       

Steps to reproduce

Run phpstan

Proposed resolution

Resolve issues

Remaining tasks

Resolve issues

User interface changes

None

API changes

None

Data model changes

None

πŸ› Bug report
Status

Fixed

Version

3.0

Component

Miscellaneous

Created by

πŸ‡¬πŸ‡§United Kingdom scott_euser

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @scott_euser
  • πŸ‡¬πŸ‡§United Kingdom scott_euser
  • Merge request !58Fix deprecations β†’ (Merged) created by scott_euser
  • Issue was unassigned.
  • Status changed to Needs review 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Here is the change record from Core https://www.drupal.org/node/3436275 β†’

  • Pipeline finished with Success
    8 months ago
    Total: 329s
    #207682
  • πŸ‡¦πŸ‡ΊAustralia jannakha Brisbane!

    @scott_euser - patch doesn't apply to 8.x-3.4 - should I test on dev branch?

  • Status changed to RTBC 7 months ago
  • πŸ‡¦πŸ‡ΊAustralia jannakha Brisbane!

    tested MR!58 on D10.3, php 8.3, on 3.x-dev@dev
    looks good!

    Thanks for your fixes!

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Great thanks! Yep MRs will always target dev branch of a module. Usually that ends up being fine for patches to the release but yeah often leads to issues. Definitely does need to keep like this as the target for the merge though otherwise it does not help the maintainers.

    In any case thanks for reviewing. Over to maintainers then!

  • Status changed to Fixed 6 months ago
  • πŸ‡§πŸ‡ͺBelgium nils.destoop

    Thx for the patch. I committed this to dev

  • πŸ‡ΊπŸ‡ΈUnited States caesius

    This seems to WSOD on Drupal 10.2.

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Probably easiest is to set:
    core_version_requirement: ^10.3 || ^11

    To prevent upgrade to latest field group when leaving drupal core outdated.

  • πŸ‡ΊπŸ‡ΈUnited States caesius

    ^ Not recommended on 3.x; see comment on other issue.

  • Pipeline finished with Failed
    6 months ago
    Total: 277s
    #242507
  • Pipeline finished with Failed
    6 months ago
    Total: 215s
    #242508
  • Status changed to Needs review 6 months ago
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Okay updated 2nd branch to have both composer.json and core_version_requirement constraints. Now as long as there are two tagged releases, composer.json will negotiate to not allow update to the latest if not 10.3+. Of course tests fail on 3456987-reapply-change-step-1 as its back to having the deprecated code with the commit revert.

    Steps for maintainer:

    1. Merge 3456987-reapply-change-step-1
    2. Tag & release
    3. Merge 3456987-reapply-change-step-2
    4. Tag & release
  • Pipeline finished with Success
    6 months ago
    Total: 277s
    #242514
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    So essentially then users will get the reverted version 8.x-3.6 (3456987-reapply-change-step-1) if they are locking themselves in to Drupal 10.2 and not doing a full composer update. Composer won't then let them get 8.x-3.7.

    Then those doing a full composer update (so also 10.3) will get 8.x-3.7 (3456987-reapply-change-step-2) because composer will allow it.

  • πŸ‡ΊπŸ‡ΈUnited States caesius

    This change cannot be applied to Field Group 3.x. This module is covered by the security advisory policy, which I haven't thoroughly read, but I assume it implies that any security updates to a module must be applicable to all supported versions of Drupal. Preventing Drupal <10.3 from receiving new updates to the module would inherently prevent any potential security releases from being applied, which would be a huge problem. Drupal 10.2 is supported until the release of 10.4, so probably until December.

    As mentioned, a new full release of this module is required for this code update. BC incompatible changes usually imply a new major version, though some like Linkit β†’ opt for maintaining separate minor versions, i.e. 6.0.x and 6.1.x. However, Field Group is still using 8.x-3.x with no capability for distinguishing minor and patch versions, so a 4.0.0 release is the only option.

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Removing deprecated code has nothing to do with security. Does not make any sense to request maintainers maintain multiple versions in order for some sites to stay on old version of core.

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Seems sensible options are:

    • Wait for December 9th when 10.2 is official also end of security support (end of support was over a month ago) and accept that tests will fail.
    • Set composer requirements as per MR 2 to stop update to incompatible version of field group if actively deciding to stay on old version of core.
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Ah I see sorry you aren't saying it's a security issue it itself, you are speaking in the hypothetical. If there is an urgent security release for 10
    2 core for field group before December 9th the maintainers can of course add back in the deprecated code and make a release.

  • πŸ‡ΊπŸ‡ΈUnited States caesius

    So you're suggesting that they release a 3.7 version that can't be installed on Drupal 10.2, and then potentially later release a 3.8 with a security fix and rollback of this code that can be installed on 10.2, and then release 3.9 with the code re-removed, composer constraint re-added, and the security fix in place?

    I think you're missing the point of semantic versioning. Field Group 8.x-3.x has 200,000 installs and is so frequently used that issues for it are sometimes filed to core πŸ› Field Groups marked as required are missing red asterisk Needs work . As such, the community needs to be confident that it is stable, and it needs to be available for all supported versions of Drupal, potentially even in-development versions such as Drupal 12.

    If this code is absolutely required for automated tests to pass then it should be added to the project, but it can't be done in a way that introduces uncertainty about which version of the module is actually compatible with which version of Drupal. I don't understand the reluctance and obstinance of module maintainers to actually release a new major version of a module when making backward incompatible updates or potentially breaking changes. If any module needs to set an example for how to properly maintain versions and apply semantic versioning, it's this one. "It's a hassle" is not an excuse to do otherwise.

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Fair points. Just trying to be considerate to maintainers time, but really I have no idea, maybe it's paid time rather than volunteer. Was ultimately just trying to be helpful to get tests passing for them again but obviously tests aren't covering old versions as became apparent here, sorry for all the hassle this caused!

  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Just looked back at the background of why I tried to fix the tests, was trying to help get πŸ› HTML5 Validation Prevents Submission in Tabs Postponed: needs info 6 year old issue over the line.

  • Status changed to Needs work 6 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    My 2c would be to create a new major (4.x, without the 8.x-), there you would set the minimum version to 10.3 and 11. That way phpstan and tests will run on the latest minor and next major and we can keep 8.x-3.x for just security updates.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    acbramley β†’ changed the visibility of the branch 3456987-revert-change-step-1 to hidden.

  • Merge request !65Use phpstan baseline for now β†’ (Closed) created by acbramley
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    In the meantime you could add a baseline to allow phpstan to pass again :) https://git.drupalcode.org/project/field_group/-/merge_requests/65

  • Status changed to Needs review 6 months ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley
  • πŸ‡¬πŸ‡§United Kingdom scott_euser

    Ignoring it is an option, just need to make sure there are adequate reminders in place. Happy to set a reminder myself to follow up in this issue Jan 2025 if that's the decision.

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    @acbramley and @scott_euser if we're all fine with that, we can surely create a 4.x branch as suggested in #23 and merge MR!65

    @scott_euser could you create a follow-up for 4.x to only support Drupal ^10.3 || ^11 and remove that ignore?

    The last question is, if we should already switch to 4.x now or fix other issues in 8.x-3.x before as I don't think that branch will get no more features then...

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    Follow-up: πŸ“Œ Fix Drupal 11 deprecations Needs work

  • Status changed to Fixed 5 months ago
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024