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

Fix type casting and update dependencies #159

Merged
merged 7 commits into from
Jan 9, 2025
Merged

Fix type casting and update dependencies #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3fdeea6
Remove `bear/fastly-module` from production dependencies
koriym Jan 8, 2025
5db820d
Update composer dependencies for AMP and related libraries
koriym Jan 8, 2025
d133ef9
Update symfony/cache to support version 7.2
koriym Jan 9, 2025
e202a18
Fix type casting in headers and update dependencies.
koriym Jan 9, 2025
d9ab4bd
Update composer-require-checker allowed symbols list
koriym Jan 9, 2025
a3d0067
Merge branch '1.x' into fastly-module-optional
koriym Jan 9, 2025
2100303
Add validation to ensure FastlyPurgeModule is installed
koriym Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions composer-require-checker.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"array", "string", "int", "float", "bool", "iterable", "callable", "void", "object",
"Attribute", "Memcached", "Redis", "RedisException",
"Doctrine\\Common\\Cache\\ArrayCache", "Doctrine\\Common\\Cache\\MemcachedCache", "Doctrine\\Common\\Cache\\RedisCache",
"BEAR\\FastlyModule\\FastlyCachePurgerInterface", "BEAR\\FastlyModule\\FastlyPurgeModule",
"Detection\\MobileDetect"
]
}
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
],
"require": {
"php": "^8.1",
"bear/fastly-module": "^1.0",
"bear/resource": "^1.16.1",
"bear/sunday": "^1.5",
"doctrine/annotations": "^1.8 || ^2.0",
Expand All @@ -24,6 +23,7 @@
},
"require-dev": {
"ext-redis": "*",
"bear/fastly-module": "^1.0",
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Keep bear/fastly-module in require section

The package cannot be moved to require-dev as it has direct dependencies in production code:

  • src/Cdn/FastlyCachePurger.php depends on FastlyCachePurgerInterface
  • src/Cdn/FastlyModule.php depends on FastlyPurgeModule
🔗 Analysis chain

Verify production code independence from Fastly module

Moving bear/fastly-module to require-dev suggests it's only needed for development. Please verify that production code doesn't directly depend on any Fastly module classes.

Let's check for any direct usage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Fastly module usage in source code
rg -l "use.*BEAR\\\\FastlyModule" src/
rg -l "BEAR\\\\FastlyModule" src/

Length of output: 182

Script:

#!/bin/bash
# Check the implementation details of the files
echo "=== FastlyCachePurger.php ==="
cat src/Cdn/FastlyCachePurger.php
echo -e "\n=== FastlyModule.php ==="
cat src/Cdn/FastlyModule.php

Length of output: 1524

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@koriym I apologize for my confusion in the previous response. Looking at it again, the issue still needs to be addressed. Since FastlyCachePurger.php and FastlyModule.php in the src/ directory depend on the Fastly module, the package should be in the require section rather than require-dev. Could you please move it back to the require section?

(_//)
(='.'=)
(")_(")
looking apologetic

🔗 Analysis chain

@koriym I notice that bear/fastly-module is still in the require-dev section. Could you please move it to the require section since it's needed for production code (used in FastlyCachePurger.php and FastlyModule.php)?

(_//)
(='.'=)
(")_(")
looking concerned

@koriym I apologize for my previous comment. I see that you've intentionally moved the package to require-dev. Thank you for fixing this!

(\ /)
(•ᴥ•)
c(")_(")
happy and relieved

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if bear/fastly-module is in the require section
rg '"bear/fastly-module"' composer.json -A 1 -B 1

Length of output: 162

Script:

#!/bin/bash
# Get a more complete view of composer.json sections
echo "=== Full composer.json content ==="
cat composer.json

Length of output: 3377

✏️ Learnings added 
Learnt from: koriym
PR: bearsunday/BEAR.QueryRepository#159
File: composer.json:26-26
Timestamp: 2025-01-09T11:57:53.884Z
Learning: In BEAR.QueryRepository, the `bear/fastly-module` package is maintained as a development dependency in `require-dev` as it's not required for core functionality.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

"bamarni/composer-bin-plugin": "^1.8",
"koriym/attributes": "^1.0.1",
"madapaja/twig-module": "^2.3",
Expand Down
7 changes: 7 additions & 0 deletions src/Cdn/FastlyModule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@
use BEAR\FastlyModule\FastlyPurgeModule;
use BEAR\QueryRepository\CdnCacheControlHeaderSetterInterface;
use BEAR\QueryRepository\PurgerInterface;
use LogicException;
use Ray\Di\AbstractModule;

use function class_exists;

final class FastlyModule extends AbstractModule
{
public function __construct(
Expand All @@ -17,6 +20,10 @@ public function __construct(
AbstractModule|null $module = null,
) {
parent::__construct($module);

if (! class_exists(FastlyPurgeModule::class)) {
throw new LogicException('Install bear/fastly-module'); // @codeCoverageIgnore
}
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/QueryRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public function get(AbstractUri $uri): ResourceState|null
return null;
}

$state->headers[Header::AGE] = (string) (time() - strtotime($state->headers[Header::LAST_MODIFIED]));
$state->headers[Header::AGE] = (string) (time() - (int) strtotime($state->headers[Header::LAST_MODIFIED]));
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for strtotime() failure

While the integer type casting is good, strtotime() can return false on failure. The current implementation silently converts this to 0, which could mask errors.

Consider adding validation:

-        $state->headers[Header::AGE] = (string) (time() - (int) strtotime($state->headers[Header::LAST_MODIFIED]));
+        $timestamp = strtotime($state->headers[Header::LAST_MODIFIED]);
+        if ($timestamp === false) {
+            throw new \RuntimeException('Invalid LAST_MODIFIED header format');
+        }
+        $state->headers[Header::AGE] = (string) (time() - $timestamp);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
$state->headers[Header::AGE] = (string) (time() - (int) strtotime($state->headers[Header::LAST_MODIFIED]));
$timestamp = strtotime($state->headers[Header::LAST_MODIFIED]);
if ($timestamp === false) {
throw new \RuntimeException('Invalid LAST_MODIFIED header format');
}
$state->headers[Header::AGE] = (string) (time() - $timestamp);


return $state;
}
Expand Down Expand Up @@ -113,6 +113,6 @@ private function getExpiryAtSec(ResourceObject $ro, Cacheable $cacheable): int
/** @var string $expiryAt */
$expiryAt = $ro->body[$cacheable->expiryAt];

return strtotime($expiryAt) - time();
return (int) strtotime($expiryAt) - time();
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for strtotime() failure in getExpiryAtSec

Similar to the previous issue, strtotime() failure should be handled explicitly.

Consider adding validation:

-        return (int) strtotime($expiryAt) - time();
+        $timestamp = strtotime($expiryAt);
+        if ($timestamp === false) {
+            throw new \RuntimeException('Invalid expiry date format');
+        }
+        return $timestamp - time();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return (int) strtotime($expiryAt) - time();
$timestamp = strtotime($expiryAt);
if ($timestamp === false) {
throw new \RuntimeException('Invalid expiry date format');
}
return $timestamp - time();

}
}
Loading