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

remove annotation for 3.X release #600

Closed
wants to merge 0 commits into from
Closed

remove annotation for 3.X release #600

wants to merge 0 commits into from

Conversation

toxicity1985
Copy link
Contributor

No description provided.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot!

i added some comments.

.github/workflows/php.yml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
src/Configuration/ConfigurationInterface.php Outdated Show resolved Hide resolved
@@ -11,7 +11,7 @@

namespace FOS\HttpCacheBundle\Http\ResponseMatcher;

use Sensio\Bundle\FrameworkExtraBundle\Security\ExpressionLanguage as SecurityExpressionLanguage;
use FOS\HttpCacheBundle\Security\ExpressionLanguage as SecurityExpressionLanguage;
Copy link
Contributor

Choose a reason for hiding this comment

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

@toxicity1985
Copy link
Contributor Author

There is 2 bundle to be updated to be comptabile with php8.2.

@toxicity1985
Copy link
Contributor Author

Do you have an idea why the rule matcher is not ok ?
Something has change but i don't found what.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks for the work!

i try to get the ball rolling with the cloudfront extension. worst case we disable the test if the component is not updated for the new version soonish.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated
@@ -35,21 +35,20 @@
"guzzlehttp/guzzle": "^7.2",
"mockery/mockery": "^1.3.2",
"monolog/monolog": "*",
"sensio/framework-extra-bundle": "^4.0 || ^5.5.1 || ^6.0",
"doctrine/annotations": "^1.11",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this?

{
/**
* @var string Regular expression to match the query string part of the request url
*/
private $queryString;

public function __construct($queryString = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

when we add this to the constructor, i'd like to remove the setter for queryString.

composer.json Outdated Show resolved Hide resolved
src/Configuration/ConfigurationInterface.php Outdated Show resolved Hide resolved
*
* @author Fabien Potencier <[email protected]>
*/
interface ConfigurationInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at how we use this in Php8AttributesListener i would prefer to do a match on $instance to see if it is one of the 3 known attribute classes and if so add it with the corresponding alias name. this interface is not really adding value anymore i think.

src/Command/BaseInvalidateCommand.php Outdated Show resolved Hide resolved
@toxicity1985
Copy link
Contributor Author

Hi,

I update the plugin this morning. I add a match function as asked but the get alias is necessary to add configuration inside the request. I juste have a question regards the invalidation path attribute. If we have a match we have to clear the path configured inside the attribute but not the route called ?

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