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

Move oembed to src #4156

Merged
merged 3 commits into from Jan 2, 2018
Merged

Conversation

MrPetovan
Copy link
Collaborator

Related to #4155

Part of #3878
Part of #4090

This is just a preparation for the work that hopefully will be agreed upon in #4155

Copy link
Collaborator

@annando annando left a comment

Choose a reason for hiding this comment

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

I guess you had tested it on your side for some time?

There are only minor issues that I found.

<?php

/*
* To change this license header, choose License Headers in Project Properties.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines confusing me. Are these place holders?

Additionally @file is missing.

require_once 'mod/proxy.php';

/**
* Description of OEmbed
Copy link
Collaborator

Choose a reason for hiding this comment

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

A little bit too generic, I think.

public static function replaceCallback($matches)
{
$embedurl = $matches[1];
$j = OEmbed::fetchURL($embedurl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't self:: be better here? (Same goes for the line below and much other places)

private static function buildXPath($attr, $value)
{
// https://www.westhoffswelt.de/blog/2009/6/9/select-html-elements-with-more-than-one-css-class-using-xpath
return "contains( normalize-space( @$attr ), ' $value ' ) or substring( normalize-space( @$attr ), 1, string-length( '$value' ) + 1 ) = '$value ' or substring( normalize-space( @$attr ), string-length( @$attr ) - string-length( '$value' ) ) = ' $value' or @$attr = '$value'";
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are several spaces in that line that could be removed to make it look better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some spaces are required are it is looking for a value in a space-separated list, but there are a several others I can indeed remove.

@MrPetovan MrPetovan mentioned this pull request Jan 1, 2018
- Add file comment in Content\OEmbed
- Add self where relevant
- Remove useless spaces from the XPath expression
@annando annando merged commit 158cb8d into friendica:develop Jan 2, 2018
@MrPetovan MrPetovan deleted the bug/4155-move-oembed-to-src branch January 2, 2018 15:52
@MrPetovan MrPetovan added this to the 3.6 milestone Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants