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.

Sign up for GitHub

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

Jump to bottom

Update phpcr-shell to Symfony 7 #224

Closed
wants to merge 10 commits into from
Closed

Update phpcr-shell to Symfony 7 #224

wants to merge 10 commits into from

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Nov 29, 2023 •
edited
Loading

Build on top of: #223

Still on Symfony 6 instead of 7:

symfony/config                     v6.4.0  Helps you find, load, combine, autofill and validate ...
symfony/console                    v6.4.0  Eases the creation of beautiful and testable command ...
symfony/dependency-injection       v6.4.0  Allows you to standardize and centralize the way obje...
symfony/event-dispatcher           v6.4.0  Provides tools that allow your application components...
symfony/finder                     v6.4.0  Finds files and directories via an intuitive fluent i...
symfony/process                    v6.4.0  Executes commands in sub-processes
symfony/translation                v6.4.0  Provides tools to internationalize your application
symfony/yaml                       v6.4.0  Loads and dumps YAML files

Following deps blocking this updates:

behat/behat       v3.13.0 requires symfony/console (^4.4 || ^5.0 || ^6.0)
phpcr/phpcr-utils 1.8.1   requires symfony/console (^2.3 || ^3.0 || ^4.0 || ^5.0 || ^6.0)
phpspec/phpspec   7.4.0   requires symfony/console (^3.4 || ^4.4 || ^5.0 || ^6.0)

TODO

  • test with symfony 7 and php 8.3 phpcr-utils#214 (release missing)
  • feat: allow symfony 7 phpspec/phpspec#1452
  • allow Symfony 7 Behat/Behat#1442
  • PHP Fatal error: Declaration of PHPCR\Shell\Serializer\NodeNormalizer::normalize($node, $format = null, array $context = []) must be compatible with Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize(mixed $object, ?string $format = null, array $context = []): ArrayObject|array|string|int|float|bool|null in /home/runner/work/phpcr-shell/phpcr-shell/src/PHPCR/Shell/Serializer/NodeNormalizer.php on line 45
  • Uncaught Error: Interface "Symfony\Component\DependencyInjection\ContainerAwareInterface" not found in /home/runner/work/phpcr-shell/phpcr-shell/src/PHPCR/Shell/Console/Command/BaseCommand.php:19

@alexander-schranz alexander-schranz mentioned this pull request Nov 29, 2023
Merged
76 tasks
dbu
dbu reviewed Nov 30, 2023
View reviewed changes
composer.json
"symfony/yaml": "^5.0 || ^6.0",
"symfony/dependency-injection": "^5.0 || ^6.0",
"symfony/expression-language": "^5.0 || ^6.0",
"phpcr/phpcr-utils": "^1.2 || ^2.0",
Copy link
Member

@dbu dbu Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

can we try the dev version for now, to avoid a broken release of phpcr-utils?

Suggested change
"phpcr/phpcr-utils": "^1.2 || ^2.0",
"phpcr/phpcr-utils": "^1.2 || ^2.0@dev",

Copy link
Contributor Author

@alexander-schranz alexander-schranz Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Did apply this in the CI to test against @dev dependencies. We still are blocked by phpspec and behat here to test fully against Symfony 7:

alexander-schranz
alexander-schranz commented Feb 19, 2024
View reviewed changes
Copy link
Contributor Author

@alexander-schranz alexander-schranz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about the bc breaks? As this is more a shell as a library I think we could ignore them?

@dbu / @dantleech

src/PHPCR/Shell/Serializer/NodeNormalizer.php Outdated
@@ -42,7 +42,7 @@ public function getNotes()
/**
* {@inheritdoc}
*/
public function normalize($node, $format = null, array $context = [])
public function normalize($node, $format = null, array $context = []): ArrayObject|array|string|int|float|bool|null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

technical a bc break but I think we could ignore it?

src/PHPCR/Shell/Serializer/NodeNormalizer.php
@@ -85,15 +85,15 @@ public function normalize($node, $format = null, array $context = [])
/**
* {@inheritdoc}
*/
public function supportsNormalization($data, $format = null)
public function supportsNormalization($data, $format = null, array $context = []): bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

technical a bc break but I think we could ignore it?

Copy link
Member

Choose a reason for hiding this comment

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

this is to adjust to symfony 7 BC breaks, right?

it would only be an issue if somebody extended the NodeNormalizer class. for this component imo that is not expected, agree that we don't do a new major version for that change.

src/PHPCR/Shell/Serializer/NodeNormalizer.php
{
return is_object($data) && $data instanceof NodeInterface;
}

/**
* {@inheritdoc}
*/
public function denormalize($data, $class, $format = null, array $context = [])
public function denormalize($data, $class, $format = null, array $context = []): mixed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

technical a bc break but I think we could ignore it?

src/PHPCR/Shell/Serializer/NodeNormalizer.php Outdated
@@ -166,7 +166,7 @@ public function denormalize($data, $class, $format = null, array $context = [])
/**
* {@inheritdoc}
*/
public function supportsDenormalization($data, $type, $format = null)
public function supportsDenormalization($data, $type, $format = null): bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

technical a bc break but I think we could ignore it

src/PHPCR/Shell/Serializer/NodeNormalizer.php
@@ -166,7 +166,7 @@ public function denormalize($data, $class, $format = null, array $context = [])
/**
* {@inheritdoc}
*/
public function supportsDenormalization($data, $type, $format = null)
public function supportsDenormalization($data, $type, $format = null, array $context = []): bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

technical a bc break but I think we could ignore it?

@dbu
Copy link
Member

dbu commented Feb 19, 2024

the ContainerAware concept has been removed in symfony 7. we should define the interface in the phpcr-shell and use that in BaseCommand and ShellApplication, at least for now.

@dbu dbu mentioned this pull request Feb 20, 2024
Merged
@dbu
Copy link
Member

dbu commented Feb 20, 2024

continue in #225

@dbu dbu closed this Feb 20, 2024
@alexander-schranz alexander-schranz mentioned this pull request May 10, 2024
Closed
96 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbu dbu dbu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexander-schranz @dbu