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

"google/recaptcha" library | PhpUnit | Symfony recipes | Travis-CI | Bugfix #202

Merged
merged 15 commits into from
May 22, 2018
Merged

Conversation

D3strukt0r
Copy link
Contributor

@D3strukt0r D3strukt0r commented May 13, 2018

Here I tried to fix following stuff:

  1. "google/recaptcha" library
    Instead of checking the reCaptcha on our its own, it will use the already given library from google.
    What was done is running $ composer require google/recaptcha and then accordingly change the code in src/Validator/Constraints/IsTrueValidator

However, by doing this the proxy functionality is gone. If this is no problem, the README.md and src/DependencyInjection/Configuration.php file have to be configured!

  1. PhpUnit
    Just a small change. In composer it now searches for the newest (and with php compatible) version of phpUnit just by using "phpunit/phpunit": "^5 || ^6 || ^7". What had to be changed were the files "extended" from \PHPUnit_Framework_TestCase to TestCase

  2. Symfony recipes
    When installing this bundle without a config file following error is generated:

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!
!!  In ArrayNode.php line 230:
!!                                                                             
!!    The child node "public_key" at path "ewz_recaptcha" must be configured.  
!!                                                                             
!!
!!
Script @auto-scripts was called via post-update-cmd

This can be prevented if the owner(!) creates a pull request for the recipes repo (https://github.com/symfony/recipes-contrib). Please read requirements. For example, one of them is that symfony/security cannot be "required"

I suggest following:
manifest.json

{
    "bundles": {
        "EWZ\\Bundle\\RecaptchaBundle\\EWZRecaptchaBundle": ["all"]
    },
    "copy-from-recipe": {
        "config/": "%CONFIG_DIR%/"
    },
    "env": {
        "EWZ_RECAPTCHA_PUBLIC_KEY": "needed",
        "EWZ_RECAPTCHA_PRIVATE_KEY": "needed"
    }
}

post-install.txt

<bg=blue;fg=white> Next for EWZRecaptchaBundle</>
    * <fg=blue>Insert</> sensitive values in your environment values

config/packages/ewz_recaptcha.yaml

ewz_recaptcha:
    public_key:  '%env(EWZ_RECAPTCHA_PUBLIC_KEY)%'
    private_key: '%env(EWZ_RECAPTCHA_PRIVATE_KEY)%'
  1. .travis.yml
    Also here was done just a minor fix. Because previously not the right Symfony versions were tested

  2. Bugfix
    Personally I was never able to use this package for the following problem:

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!
!!  In ParameterBag.php line 100:
!!                                                                       
!!    You have requested a non-existent parameter "templating.engines".  
!!                                                                       
!!
!!
Script @auto-scripts was called via post-update-cmd

The problem lies in src/DependencyInjection/EWZRecapchtaExtension.php
I couldn't find a better solution than following:

$templatingEngines = $container->hasParameter('templating.engines')
            ? $container->getParameter('templating.engines')
            : array('twig');

So this should be definitely reviewed!

6. php version compatibility reduced to >=5.3.9
As Symfony 2.8 requires min php 5.3.9 it would make sense to do the same for this project. The only features that can't be used is short array creation [], instead array() has to be used and the constant ::class has to be changed with the whole namespace\class.
PhpUnit ->createMock started on PhpUnit 5 (php 5.6+). Maybe use https://github.com/mockery/mockery ?

@excelwebzone
Copy link
Owner

@D3strukt0r Thanks for creating the repo. Since the proxy is required by other users, I suggest changing the code to support both options. Also, don't modify the config settings only the README, PHP and TWIG files, unless something fails when running test or required for your changes. Thanks again.

@D3strukt0r
Copy link
Contributor Author

D3strukt0r commented May 18, 2018

To support proxy I need to create a copy of ReCaptcha\RequestMethod\Post (https://github.com/google/recaptcha/blob/master/src/ReCaptcha/RequestMethod/Post.php) in which I could put the code for it, question is; where do you want me to put the file @excelwebzone ? Maybe ./Extension/ReCaptcha/RequestMethod/ProxyPost.php?

@D3strukt0r
Copy link
Contributor Author

So, basically I'm finished. Everything should work just like before

@excelwebzone excelwebzone merged commit be0488f into excelwebzone:master May 22, 2018
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.

2 participants