[FacebookBridge] Handle mobile links and standardise host validation

[SpangleLabs]

A simple little change to standardise host validation between user and group links, and allow mobile links, which should close #1782 and avoid issues like that again in the future.

I'm not sure whether this is enough to justify a hacktoberfest-accepted label, but I would be quite happy if it did. I've had my eye on some things here for hacktoberfest this year, but more than I expected are blocked by #1170

[em92]

Hi, @joshcoales !

I'm not sure whether this is enough to justify a hacktoberfest-accepted label, but I would be quite happy if it did.

I will add this label after accepting PR.

[em92]

Nice idea about unifying checks.

Instead of using if statement for checking validity of host, I suggest validateHost to raise client error (returnClientError) instead of returning true/false. Also you would not need to duplicate error message.

[em92]

Also make sure that

phpcs bridges/FacebookBridge.php --standard=phpcs.xml --warning-severity=0 --extensions=php -p

does not return errors.

[SpangleLabs]

Instead of using if statement for checking validity of host, I suggest validateHost to raise client error (returnClientError) instead of returning true/false. Also you would not need to duplicate error message.

Sounds good, I've pushed that now!

phpcs bridges/FacebookBridge.php --standard=phpcs.xml --warning-severity=0 --extensions=php -p

Ah, whoops, had spaces instead of tabs.
Also my IDE does raise a few warnings, so I was wondering about whether to clean up the whole thing, or just to keep the scope of the PR narrow. I can clean up other stuff in there if you want

[SpangleLabs]

Here's a commit with the whole FacebookBridge class neatened up, which shushes all the phpcs warnings. Shall I put that on this branch too?
I'm a little rusty on PHP, so some of it might not be ideal

Also, would it be helpful if I made another PR, adding type hints and neatening stuff up more generally?

EDIT: Helps if I actually link it:
SpangleLabs@9900f52

[em92]

@joshcoales, please rebase your PR in order for Travis CI to check your commit.

Also, would it be helpful if I made another PR, adding type hints and neatening stuff up more generally?

Checked that commit. In general, all changes must be discussed individually, if not covered by linting. It can take a lot of time. So I suggest not to discuss it at all.

[SpangleLabs]

Checked that commit. In general, all changes must be discussed individually, if not covered by linting. It can take a lot of time. So I suggest not to discuss it at all.

That makes sense to me. Any other changes needed on this PR?

[em92]

Any other changes needed on this PR?

As I mentioned above - rebase your PR

[SpangleLabs]

As I mentioned above - rebase your PR

Sorry about that, I missed that somehow.

I have rebased this branch now.

EDIT: Looks like Travis has jammed though? Spawned two builds, but both link to the same place, which has passed.

[em92]

Thanks for contribution. You can check other facebook issues to fix. At the moment, facebook groups don't seem to be working.

[SpangleLabs]

Sounds like a plan! I've got a few other things here I've got an eye on too. (A few fixes I would like to make, but also some PRs, like #1758 )

How would you feel about automated external integration tests?
Like, would it be helpful if I wrote up a test that can be ran daily or weekly, to ensure the bridge still works?

[em92]

Like, would it be helpful if I wrote up a test that can be ran daily or weekly, to ensure the bridge still works?

If some bridge does stops working, its user will open issue here. So, there is no need for those integration tests

[SpangleLabs]

At the moment, facebook groups don't seem to be working.
Looks like facebook has gone fully javascript on that, and the page only returns the first post until javascript requests start being made.
However, it looks like touch.facebook.com can be used, like FB2Bridge uses.

It seems like switching to that mechanism would be the best way to make things work. Would that deprecate FB2Bridge?

[em92]

Actually I don't know why FB2 bridge exists as separate bridge for Facebook.

@teromene, hi! Do you have and idea why it was made like this?

[teromene]

The existence of FB2 is purely done because of historical reasons:
When the first Facebook bridge was released, it had an excellent HTML parsing.
Rapidly, we had the captcha problem, but only on few users.
We found that using another Facebook website worked for these users, but parsing was way harder, and the result worse.
We therefore decided to create two distinct bridges to cater to everyone's need.