Skip to content

[ticket/17618] Cure Symfony 7 (and PHP 8.2+) deprecation warnings#6934

Open
iMattPro wants to merge 14 commits intophpbb:masterfrom
iMattPro:ticket/17618
Open

[ticket/17618] Cure Symfony 7 (and PHP 8.2+) deprecation warnings#6934
iMattPro wants to merge 14 commits intophpbb:masterfrom
iMattPro:ticket/17618

Conversation

@iMattPro
Copy link
Copy Markdown
Member

@iMattPro iMattPro commented Mar 2, 2026

Checklist:

  • Correct branch: master for new features; 3.3.x for fixes
  • Tests pass
  • Code follows coding guidelines: master and 3.3.x
  • Commit follows commit message format

Tracker ticket:

https://tracker.phpbb.com/browse/PHPBB-17618

@iMattPro iMattPro changed the title [ticket/17618] Cure all Symfony 7 deprecation warnings [ticket/17618] Cure all Symfony 7 and PHP 8.2+ deprecation warnings Mar 3, 2026
@iMattPro iMattPro changed the title [ticket/17618] Cure all Symfony 7 and PHP 8.2+ deprecation warnings [ticket/17618] Cure Symfony 7 (and PHP 8.2+) deprecation warnings Mar 3, 2026
#[\AllowDynamicProperties]
class acp_captcha
{
var $u_action;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to see this things updated to public

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's beyond the scope of this PR, and also, the ACP probably needs such a massive overhaul, that those sort of things should be saved for that job. The ACP files are basically PHP 4 language-level code 😂


namespace phpbb\template\twig\node;

#[\Twig\Attribute\YieldReady]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely is not yield ready yet since we're using things like ob_start()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this #[YieldReady]; replaced ob_start()/ob_get_clean() with the CaptureNode pattern:

CoreExtension::captureOutput(...) in echo mode

implode('', iterator_to_array(...)) in yield mode


namespace phpbb\template\twig\node;

#[\Twig\Attribute\YieldReady]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Highly doubt that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made #[YieldReady]
Conditionally emits yield from ->unwrap()->yield() vs ->display() based on $compiler->getEnvironment()->useYield() - same pattern Twig uses internally


namespace phpbb\template\twig\node;

#[\Twig\Attribute\YieldReady]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, don't just add YieldReady because things need to be YieldReady for twig 4.0

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be YieldReady. Produces no template output at all — only adds to an asset bag


namespace phpbb\template\twig\node;

#[\Twig\Attribute\YieldReady]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link
Copy Markdown
Member Author

@iMattPro iMattPro Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Produces no template output — only adds to an asset bag. Needs #[YieldReady] explicitly because PHP attributes aren't inherited


namespace phpbb\template\twig\node;

#[\Twig\Attribute\YieldReady]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link
Copy Markdown
Member Author

@iMattPro iMattPro Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Produces no template output — only adds to an asset bag. Needs #[YieldReady] explicitly because PHP attributes aren't inherited


namespace phpbb\template\twig\node;

#[\Twig\Attribute\YieldReady]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already yield-safe (only sets vars, delegates to yield-ready parent \Twig\Node\IncludeNode::compile())

$only = false;
$ignoreMissing = false;

if ($this->parser->getStream()->nextIf(\Twig\Token::NAME_TYPE, 'with'))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use local variable for stream, same as current IncludeTokenParser

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, probably nicer to also just put this into a separate function

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a local variable for stream now (also found another class that missed using a local variable for stream

@iMattPro
Copy link
Copy Markdown
Member Author

@marc1706 I went through each of the twig node files, checked them for Yield readiness, and made them Yield ready where necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants