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

Feature symfony7 update #306

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

snipershady
Copy link

Now compliant with Symfony 7.0

@excelwebzone
Copy link
Owner

@snipershady please run the check and fix the issues before I can merge it. Thanks.

@snipershady
Copy link
Author

@snipershady please run the check and fix the issues before I can merge it. Thanks.

Michael I've fixed, now all PHPunit tests are succesfully completed

OK (23 tests, 99 assertions)

@excelwebzone
Copy link
Owner

Nope, still not passing.. https://github.com/excelwebzone/EWZRecaptchaBundle/actions/runs/7359000381/job/20033164385?pr=306

@snipershady
Copy link
Author

Oo I forgot what php < 7.3 does not support strict type declartion for attributes...

let me fix it again with legacy compatibility

@excelwebzone
Copy link
Owner

@snipershady
Copy link
Author

snipershady commented Dec 29, 2023

Still not working.. https://github.com/excelwebzone/EWZRecaptchaBundle/actions/runs/7359071392/job/20033299482?pr=306

because we cannot run test with Symfony 7.0 on PHP < 8.2

I think we should create a new branch of the package with PHP 8.2 strict compatibility, if we want to run this bundle on symfony 7.

composer.json of symfony
"require": { "php": ">=8.2", "ext-ctype": "*", "ext-iconv": "*", ... ... }

what do you think about?

@snipershady
Copy link
Author

I've updated the arraylist of version and tests for CI

🤞

@excelwebzone
Copy link
Owner

Sorry. Still not working.. Right now I don't have a lot of time to spend on this.. but once I'd I will try to fix your PR

@snipershady
Copy link
Author

Sorry. Still not working.. Right now I don't have a lot of time to spend on this.. but once I'd I will try to fix your PR

I've rollback to some backward legacy compatibility for the deprecated PHP version
let's figure it out if this is the final shot

@excelwebzone
Copy link
Owner

Still not working. I don't think you can convert to PHP7 unless you remove all previous versions. Sorry :-(

@snipershady
Copy link
Author

Still not working. I don't think you can convert to PHP7 unless you remove all previous versions. Sorry :-(

it is on you, I do not want to change compatibilty of the package with legacy versions, but it seams to be necessary.
I've done a fork only with current version of PHP and Symfony to upgrade my symfony softwares, and it works fine. If you accept to change the backward compatibility with ancient php version and ancient symfony version, I will be glad to prepare a new PR.

@@ -26,7 +27,7 @@ public function __construct(array $options = null, string $message = null, strin
/**
* @return string|string[]
*/
public function getTargets()
public function getTargets(): array|string
Copy link
Contributor

Choose a reason for hiding this comment

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

Union Type is not supported by PHP 7.x, please remove it

@@ -37,12 +38,11 @@ class IsTrueValidator extends ConstraintValidator
*
* @var bool
*/
protected $verifyHost;
protected bool $verifyHost;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typed properties are supported by PHP 7.4+
You need to remove bool to keep compatibility with lower PHP version

* @param ContainerBuilder $container
* @return array|string|false
*/
private function getTwigFormResources(ContainerBuilder $container): array|string|false
Copy link
Contributor

Choose a reason for hiding this comment

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

Union types to remove


/**
* The reCAPTCHA server URL.
*
* @var string
*/
protected $recaptchaApiServer;
protected string $recaptchaApiServer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please removes all property types from that class

if (forms.length) {
var recaptchaSubmitEvent = document.createEvent('Event');
let recaptchaSubmitEvent = document.createEvent('Event');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unrelated to Symfony 7

@Seb33300
Copy link
Contributor

Seb33300 commented Jan 9, 2024

This PR can be closed

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.

None yet

3 participants