Prevent PHP notices if ad dates are not set #77
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AWPCP_Meta
participant Properties
User->>AWPCP_Meta: Request date
AWPCP_Meta->>Properties: Check published-time
AWPCP_Meta->>Properties: Check modified-time
alt If properties are empty
AWPCP_Meta-->>User: Return original date
else
AWPCP_Meta-->>User: Return converted date
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/class-awpcp-meta.php (2)
261-263: LGTM! Consistent null check implementation.The null check for 'modified-time' follows the same pattern as
get_the_date, providing a robust solution for preventing PHP notices.Consider adding unit tests to verify the behavior when:
- dates are null
- dates are empty strings
- dates are valid
Would you like me to help generate these test cases?
249-251: Consider extracting date validation logic.Both methods share similar null-checking logic. Consider extracting this into a private helper method to improve maintainability and reduce code duplication.
Here's a suggested implementation:
+ private function get_formatted_date($date_value, $original_date, $format) { + if (empty($date_value)) { + return $original_date; + } + return mysql2date($format, $date_value); + } public function get_the_date($the_date, $format = '') { if (!$format) { $format = get_option('date_format'); } - if (empty($this->properties['published-time'])) { - return $the_date; - } - return mysql2date($format, $this->properties['published-time']); + return $this->get_formatted_date($this->properties['published-time'], $the_date, $format); } public function get_the_modified_date($the_date, $format) { if (!$format) { $format = get_option('date_format'); } - if (empty($this->properties['modified-time'])) { - return $the_date; - } - return mysql2date($format, $this->properties['modified-time']); + return $this->get_formatted_date($this->properties['modified-time'], $the_date, $format); }Also applies to: 261-263
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
frontend/class-awpcp-meta.php(2 hunks)
🔇 Additional comments (1)
frontend/class-awpcp-meta.php (1)
249-251: LGTM! Null check prevents PHP notices.
The added null check for 'published-time' properly prevents PHP notices while maintaining backward compatibility by returning the original date.
Let's verify the usage of this method and potential null values:
✅ Verification successful
The null check is correctly implemented and necessary
The verification shows that:
- The
published-timeproperty is populated viaawpcp_get_ad_share_info()in thefind_current_listing()method - This property is used in both OpenGraph meta tags and date formatting
- The null check prevents PHP notices when the listing data is not found or dates are not set
The implementation is safe because:
- It maintains backward compatibility by returning the original date
- It's consistent with the error handling pattern in the codebase
- The property can legitimately be empty when listing data is not found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to get_the_date to understand the impact
rg "get_the_date" --type php
# Search for assignments to published-time property to understand when it might be null
ast-grep --pattern 'published-time'
Length of output: 263
Script:
#!/bin/bash
# Let's check where the properties array is set and how published-time is populated
rg "'properties'" --type php -B 2 -A 2
# Also check for any date-related property assignments
rg "->properties\[.*time" --type php -B 2 -A 2
Length of output: 1118
Script:
#!/bin/bash
# Let's try a different approach to find property assignments
rg "\\\$this->properties\[" --type php -B 2 -A 2
# Also check the constructor or initialization of the Meta class
ast-grep --pattern 'class AWPCP_Meta {
$$$
}'
Length of output: 2514
Script:
#!/bin/bash
# Let's check for the constructor or set_properties method
rg "function __construct|function set_properties" --type php -B 2 -A 10 frontend/class-awpcp-meta.php
# Also check where this class is instantiated
rg "new AWPCP_Meta" --type php -B 2 -A 2
Length of output: 3215
Script:
#!/bin/bash
# Let's check the configure method where properties might be set
rg "function configure" --type php -B 2 -A 20 frontend/class-awpcp-meta.php
# Also check for any listing-related methods that might set these properties
rg "set.*listing|get.*listing" --type php -B 2 -A 10 frontend/class-awpcp-meta.php
Length of output: 6487
|
Thanks @shervElmi! |
fixes https://github.com/Strategy11/awpcp/issues/3174
This PR prevents calling on null notices for OG meta-date formatting methods.
If the check fails we return the stored value of the date which in my tests will still be valid for search engines to pass.