Problem/Motivation

The use of Twig_SimpleFilter in /src/Twig/Extension/FieldValueExtension.php is deprecated and should be replaced with Twig\TwigFilter.

Twig_SimpleFilter was deprecated in Twig 2, and is no longer available in Twig 3, which is used in Drupal 10. The new class is available also in Twig 2, and is a simple replacement of the class name, as the Twig_SimpleFilter in Twig 2 is an alias of TwigFilter.

Likewise, the Twig_Extension has been deprecated and replaced with Twig\Extension\AbstractExtension

Comments

jcnventura created an issue. See original summary.

immaculatexavier’s picture

Status: Active » Needs review
StatusFileSize
new929 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 3310839-2.patch, failed testing. View results

sutharsan’s picture

Status: Needs work » Postponed (maintainer needs more info)

Will the module with this patch be compatible with Drupal 8.0?

jcnventura’s picture

Status: Postponed (maintainer needs more info) » Needs work

@Sutharsan, unfortunately, no. Drupal 8 used Twig 1, which didn't have this new class. But it is impossible to support Twig 3 / Drupal 10, Twig 2 / Drupal 9 and Twig 1 / Drupal 8 at the same time. The only way would be through a check if the class exists or not and to that condition to change between Twig 1/2 code, and Twig 2/3 code.

However, Drupal 8 is no longer supported, and modules should focus on supporting Drupal 10 instead of ^8 || ^9 || ^10.

In any case the patch above is missing a full class path (\Twig\TwigFilter) or a use statement.

sutharsan’s picture

I’m fine with limiting the use of the
module to D9+. That requires a new branch, and this patch should include the required changes to the info file.

immaculatexavier’s picture

Status: Needs work » Needs review
StatusFileSize
new2.93 KB
new1.94 KB

Addressed #5 - Fixed deprecation compatibility with D9+, and addressed #6 by limiting the module's use to D9+ against #2.

Status: Needs review » Needs work

The last submitted patch, 7: 3310839-7.patch, failed testing. View results

immaculatexavier’s picture

Status: Needs work » Needs review
StatusFileSize
new2.83 KB
new553 bytes

Tried to fix the failed issues against #7

jcnventura’s picture

Title: Remove deprecated Twig_SimpleFilter usage » Remove deprecated Twig_SimpleFilter and Twig_Extension usage
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Drupal 10 compatibility

Well, seems like this is now the Drupal 10 update issue, as such let's change it, and add the new bug I found: the \Twig_Extension is also not part of Twig 3, and has been replaced with:

+use Twig\Extension\AbstractExtension;

-class FieldValueExtension extends \Twig_Extension {
+class FieldValueExtension extends AbstractExtension {
jcnventura’s picture

Status: Needs work » Needs review
StatusFileSize
new642 bytes
new2.94 KB
sutharsan’s picture

Status: Needs review » Reviewed & tested by the community
jcnventura’s picture

Needs a re-roll after #3295680: Drupal Coding Standards

jcnventura’s picture

StatusFileSize
new2.96 KB

This is the re-roll of #11. Can't create an interdiff easily, but the only reason the patch didn't apply is that the coding standards issue fixed a double space before an '=' sign.

The patch in #11 was maintaining that error, and fixing that and removing a blank line at the start of that function were the only changes in this patch.

jcnventura’s picture

StatusFileSize
new3.11 KB
b0b’s picture

name: Twig Field Value test
type: module
description: 'Provides test functionality for Twig Field Value.'
hidden: true
core: 8.x
dependencies:
  - twig_field_value

One more error being thrown... core: 8.x needs to be core_version_requirement: ^9 || ^10. Seems to be working after that.

jcnventura’s picture

Status: Reviewed & tested by the community » Needs work

Regarding #16, the proper fix should be defining that module to be part of the "Testing" package, and not specifying any core or core_version_requirement at all.

jcnventura’s picture

Status: Needs work » Needs review
StatusFileSize
new569 bytes
new3.67 KB

Now fixing also #16, as I described above.

sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

... the proper fix should be defining that module to be part of the "Testing" package, and not specifying any core or core_version_requirement at all.

I did not know that, but I see some respected modules to use this approach too. This combined with a 'green' test result, I think it is RTBC again.

jcnventura’s picture

Tested now with D10 as well, all green.

As to the testing package and no "core_version_requirement" it is documented here: https://www.drupal.org/node/3070687#test

Please commit this and tag a new release, since version 2.0.1 declares itself as Drupal 10 compatible, but it really isn't without these changes.

  • jcnventura authored 13e9d0a0 on 2.0.x
    Issue #3310839 by jcnventura, immaculatexavier: Remove deprecated...
sutharsan’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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