Skip to content

Copy the regexes to the public interface #436

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

Jump to bottom
Merged
MorrisJobke merged 1 commit into from
Jul 19, 2016
Merged

Copy the regexes to the public interface #436

MorrisJobke merged 1 commit into from
Jul 19, 2016

Conversation

@nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jul 18, 2016

No description provided.

@nickvergessen nickvergessen added this to the Nextcloud Next milestone Jul 18, 2016
@mention-bot
Copy link

mention-bot commented Jul 18, 2016

@nickvergessen, thanks for your PR! By analyzing the annotation information on this pull request, we identified @LukasReschke, @DeepDiver1975 and @tanghus to be potential reviewers

@MorrisJobke
Copy link
Member

MorrisJobke commented Jul 18, 2016

👍

@icewind1991
Copy link
Member

icewind1991 commented Jul 18, 2016

Wouldn't it be better to have an isUserAgent method (which accepts IRequest::ANDROID, etc) instead of exposing the implentation details of detection

@nickvergessen
Copy link
Member Author

nickvergessen commented Jul 18, 2016

We have a method isUserAgent which accepts an array of regexes. The problem is, you don't have the regexes publicly available to use them on this method.

I would also only consider the name public api, not the actual value?

@MorrisJobke
Copy link
Member

MorrisJobke commented Jul 18, 2016

I would also only consider the name public api, not the actual value?

Could this be done by an abstract constant? Is this possible?

ChristophWurst
ChristophWurst reviewed Jul 18, 2016
View reviewed changes
lib/private/AppFramework/Http/Request.php
// Android Chrome user agent: https://developers.google.com/chrome/mobile/docs/user-agent
const USER_AGENT_ANDROID_MOBILE_CHROME = '#Android.*Chrome/[.0-9]*#';
const USER_AGENT_FREEBOX = '#^Mozilla/5\.0$#';
const REGEX_LOCALHOST = '/^(127\.0\.0\.1|localhost)$/';
Copy link
Member

@ChristophWurst ChristophWurst Jul 18, 2016

Choose a reason for hiding this comment

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

I know this is unrelated to your PR, but what about IPv6?

Copy link
Member

@MorrisJobke MorrisJobke Jul 18, 2016

Choose a reason for hiding this comment

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

Open an ticket ;)

@ChristophWurst
Copy link
Member

ChristophWurst commented Jul 18, 2016

Could this be done by an abstract constant? Is this possible?

No, according to http://php.net/manual/en/language.oop5.interfaces.php:

It's possible for interfaces to have constants. Interface constants works exactly like class constants except they cannot be overridden by a class/interface that inherits them.

@ChristophWurst ChristophWurst mentioned this pull request Jul 18, 2016
Closed
@nickvergessen
Copy link
Member Author

nickvergessen commented Jul 19, 2016

I mean we could also add methods that then use isUserAgent(self::CONST) but I prefer those 3 consts over 3 new methods.

@LukasReschke
Copy link
Member

LukasReschke commented Jul 19, 2016

👍

@MorrisJobke MorrisJobke merged commit 544c3c1 into master Jul 19, 2016
@MorrisJobke MorrisJobke deleted the public-regex-to-match-our-clients branch July 19, 2016 09:35
@snyk-bot snyk-bot mentioned this pull request Jun 10, 2022
Open
@AveryPalmer AveryPalmer mentioned this pull request Dec 1, 2023
Open
@kaocher82 kaocher82 mentioned this pull request Dec 1, 2023
Open
@gisforgirard gisforgirard mentioned this pull request Dec 11, 2023
Open
R0Wi pushed a commit to R0Wi/server that referenced this pull request Nov 22, 2025
@bigcat88
Fix: check healthcheck and heartbeat after ExApp container start (nex…
648fec3 
…tcloud#436)

After the previous PR(nextcloud#430) which added stopping application containers
feature when shutting down, as @andrey18106 correctly noted during our
1:1 conversation:

we cannot rely on the application inside the docker container to be
ready to process the request at the moment the docker container is
turned on.

This PR introduces handling of it by checking health-check and
heartbeat.

Signed-off-by: bigcat88 <bigcat88@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Milestone

Nextcloud 10.0

Development

Successfully merging this pull request may close these issues.

7 participants

@nickvergessen @mention-bot @MorrisJobke @icewind1991 @ChristophWurst @LukasReschke