-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[GoogleGroupsBridge] Add new bridge for Google Groups #2451
Conversation
f3432dd
to
ee3d47d
Compare
} | ||
|
||
protected function provideWebsiteContent() { | ||
return defaultLinkTo(getContents($this->getSourceUrl()), self::URI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why this is necessary is because Google groups defines the base as the root (<base href="https://groups.google.com/">
) while XPathAbstract uses the source url as the base (for example https://groups.google.com/a/mozilla.org/g/announce
), which leads to duplication of a/mozilla.org/g
in all the relative urls used in the document if this is not used to correctly convert the relative links first.
bridges/GoogleGroupsBridge.php
Outdated
} | ||
} | ||
|
||
const URL_REGEX = '/^https:\/\/groups.google.com(?:\/a\/(?<account>\S+))?(?:\/g\/(?<group>\S+))/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use # as delimiter instead of /
🤖 Pull request artifacts
|
Thanks, fixed |
@Bockiii Not sure what's wrong with the CI, see actual content: |
Only the group is mandatory, the account is not. Thats why the tester only picks up the config for group and not for account. The tester-link shows "announce - Google Groups" while your screenshot shows "announce@mozilla.org - Google Groups". Basically, the problem is that just by using all mandatory fields (group in this case), the bridge will not create a valid feed. |
ah, I see. This is a quick fix since both https://groups.google.com/g/governance and https://groups.google.com/a/mozilla.org/g/governance/ exist |
LGTM |
Closes #2240