Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix table render of DiInfoCommand #38813

Closed
wants to merge 1 commit into from

Conversation

jissereitsma
Copy link
Contributor

@jissereitsma jissereitsma commented Jun 7, 2024

When running the DiInfoCommand under Magento 2.4.7 and PHP 8.3, line 96 mentions to pass the output of $paramsTable->render() to the $output. However, because of typing issues, this causes a Fatal Error. According the Symfony docs (since a long time) the render() method is already giving output, so the output should not be passed to $output->writeln() anyway. This PR fixes things.

Resolved issues:

  1. resolves [Issue] Fix table render of DiInfoCommand #38820: Fix table render of DiInfoCommand

When running the DiInfoCommand under Magento 2.4.7 and PHP 8.3, line 96 mentions to pass the output of `$paramsTable->render()` to the `$output`. However, because of typing issues, this causes a Fatal Error. According the Symfony docs (since a long time) the `render()` method is already giving output, so the output should not be passed to `$output->writeln()` anyway. This PR fixes things.
Copy link

m2-assistant bot commented Jun 7, 2024

Hi @jissereitsma. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:
  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@engcom-Charlie
Copy link
Contributor

@magento create issue

@engcom-Charlie engcom-Charlie added the Priority: P3 May be fixed according to the position in the backlog. label Jun 11, 2024
@hostep
Copy link
Contributor

hostep commented Jun 26, 2024

We already have a similar PR with the same change and an extra one: #38747 which fixes #38740
I think #38747 is more complete then this one and it's probably better to move that one forward?

@jissereitsma
Copy link
Contributor Author

Indeed this is a duplicate. I would favor #38747 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P3 May be fixed according to the position in the backlog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue] Fix table render of DiInfoCommand
3 participants