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

Introduce new method to check if a segment is valid and available for a given site #23034

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Feb 12, 2025

Description:

This PR introduces a new static method to easily allow checking if a segment is actually valid and available.

In addition this PR changes the nasty warning, that is being logged in development mode when a certain segment is created without start and end date.

This warning was actually meant to ensure that start and end dates are set when the segment is used to create a sql query.

Therefor this PR ensures that the warning is now only logged when the object is used to generate a query, rather upon construction.

Review

@sgiehl sgiehl added the Needs Review PRs that need a code review label Feb 12, 2025
@sgiehl sgiehl added this to the 5.3.0 milestone Feb 12, 2025
@sgiehl sgiehl requested a review from mneudert February 12, 2025 15:30
@@ -586,6 +608,10 @@ public static function getSegmentHash($definition)
*/
public function getSelectQuery($select, $from, $where = false, $bind = array(), $orderBy = false, $groupBy = false, $limit = 0, $offset = 0, $forceGroupBy = false)
{
if (Development::isEnabled() && !empty($this->missingDatesException)) {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it does make sense to not show the warning whenever a segment is created but then also not used.

However, would it also make sense to include a double stacktrace? One for "segment created at" and one for "segment used at"? Depending on the code it may be difficult to find out where it was used, if you only get the information where it was created, unless you know how to reliably reproduce the error and get a breakpoint triggered 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll change that so it will include both stacktraces

@sgiehl sgiehl requested a review from mneudert February 14, 2025 08:41
@mneudert mneudert merged commit a1d667a into 5.x-dev Feb 14, 2025
21 of 26 checks passed
@mneudert mneudert deleted the segmentwarning branch February 14, 2025 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants