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

LinkResolver class usage #18

Closed
tremby opened this issue Feb 18, 2014 · 10 comments · Fixed by #19
Closed

LinkResolver class usage #18

tremby opened this issue Feb 18, 2014 · 10 comments · Fixed by #19

Comments

@tremby
Copy link
Contributor

tremby commented Feb 18, 2014

How is the LinkResolver abstract class supposed to be used? I have tried passing a closure as $linkResolver to asHtml on some structured text and this works, but for more nicely structured code I would have thought it'd make sense to write my own resolver class. I've tried this:

class Resolver extends \Prismic\LinkResolver {
    public function resolve($documentLink) {
        return '/' . $documentLink->getType() . '/' . $documentLink->getSlug();
    }
}
...
$resolver = new Resolver;
var_dump($results[0]->get('content-page.post')->asHtml($resolver);

and I'm getting an error on StructuredText.php line 175 saying "Function name must be a string", where it tries to do $url = $linkResolver ? $linkResolver($span->getLink()) : '';

@rudyrigot
Copy link
Contributor

Hey,

I think you'll find all of the answers to your questions and more by looking at how things are done in our plain PHP starter project (we also have one for PHP Symfony if you prefer).

Writing your own class sounds like the cleanest way to do it in an advanced way, that's the way we've done it ourselves, except we've also used a Routes class to organize our routes.

(By the way, have you noticed that your code's last line is missing a closing parenthesis?)

Does this bring clarity to your issue? If not, no problem, I'll look further with you.

@tremby
Copy link
Contributor Author

tremby commented Feb 18, 2014

Missing parenthesis is a typo in the issue, not present in my code.

The code you link to seems to be doing things the same way as what I have above, in that it extends the LinkResolver class, makes an instance of it and then passes that instance to the asHtml method. I don't see what I'm doing wrong here.

Above I'm calling asHtml on a StructuredText instance and I'm getting the "Function name must be a string". That error makes sense -- look at the code which is trying to run.

https://github.com/prismicio/php-kit/blob/master/src/Prismic/Fragment/StructuredText.php#L175

An object instance is not callable. You can't do $linkResolver = new Resolver; return $linkResolver($documentLink);

I tried calling asHtml on the whole document instead, with var_dump($results[0]->asHtml($resolver));. This time it runs, but the resolve method of my Resolver isn't actually being called at all (I have put an xdebug_break() call in there which isn't tripped) and as such the anchor element comes back with an empty href attribute.

Now, if I try to run asHtml on the Document instance with a closure passed as $linkResolver instead of my Resolver instance, I get the same behaviour -- the closure is not called and the link href is empty.

@rande
Copy link
Contributor

rande commented Feb 18, 2014

My 2c on the subject. Just add a test case with the json and the code ;)

Thomas Rabaix
http://rabaix.net - http://sonata-project.org
On Feb 18, 2014 10:13 PM, "Bart Nagel" notifications@github.com wrote:

Missing parenthesis is a typo in the issue, not present in my code.

The code you link to seems to be doing things the same way as what I have
above, in that it extends the LinkResolver class, makes an instance of it
and then passes that instance to the asHtml method. I don't see what I'm
doing wrong here.

Above I'm calling asHtml on a StructuredText instance and I'm getting the
"Function name must be a string". That error makes sense -- look at the
code which is trying to run.

https://github.com/prismicio/php-kit/blob/master/src/Prismic/Fragment/StructuredText.php#L175

An object instance is not callable. You can't do $linkResolver = new
Resolver; return $linkResolver($documentLink);

I tried calling asHtml on the whole document instead, with
var_dump($results[0]->asHtml($resolver));. This time it runs, but the
resolve method of my Resolver isn't actually being called at all (I have
put an xdebug_break() call in there which isn't tripped) and as such the
anchor element comes back with an empty href attribute.

Now, if I try to run asHtml on the Document instance with a closure
passed as $linkResolver instead of my Resolver instance, I get the same
behaviour -- the closure is not called and the link href is empty.


Reply to this email directly or view it on GitHubhttps://github.com//issues/18#issuecomment-35434631
.

@rudyrigot
Copy link
Contributor

Hm, there definitely seems to be something wrong...

If you feel like fixing the bug, feel free, we'd love to look at it and merge it quickly. Otherwise, how to design and use the LinkResolver class is very language-dependent, so I'd better assign this to a more experienced PHP developer than myself on the prismic.io team.

dohzya added a commit to dohzya/php-kit that referenced this issue Feb 19, 2014
The LinkResolver is now an object (with its own
class) thus it should not be used as a function.

Should fix prismicio-community#18
@dohzya
Copy link
Contributor

dohzya commented Feb 19, 2014

Sounds like we missed this case. I added a fix and a test.

@rudyrigot merge the PR if it's ok for you :-)

@tremby
Copy link
Contributor Author

tremby commented Feb 19, 2014

Okay, thanks, @dohzya, that's an improvement.

But two issues remain:

First, I still can't get the resolver to run on a document object. Here's my code.

class Resolver extends \Prismic\LinkResolver {
    public function resolve($documentLink) {
        echo "\nresolve function running\n";
        return '/' . $documentLink->getType() . '/' . $documentLink->getSlug();
    }
}

Route::get('hello', function() {
    $results = Prismic::get('[[:d = any(document.type, ["content-page"])]]');
    $resolver = new Resolver;
    header("Content-Type: text/plain");
    echo "\n\npost HTML:\n";
    echo $results[0]->get('content-page.post')->asHtml($resolver);
    echo "\n\ndocument HTML:\n";
    echo $results[0]->asHtml($resolver);
    exit;
});

Prismic is a wrapper class to the SDK I wrote. It's doing the query and returning the results.

Here's the output I get:

post HTML:

resolve function running

resolve function running
<p>Link to <a href="/things-to-do/test-post-with-image-issue">other documents</a>.</p>

document HTML:
<section data-field="content-page.title"><h1>Test document 1</h1></section><section data-field="content-page.post"><p>Link to <a href="">other documents</a>.</p></section>

As you can see, the resolver runs (not sure why twice) for the asHtml method of the post structured text object, and the link comes out just fine. But for the document object the resolver does not run and I get an empty link href.

I will post the other issue separately -- it is in #20.

dohzya referenced this issue in dohzya/php-kit Feb 20, 2014
LinkResolver was only provided to DocumentLink, so
links nested in a StructuredText (for instance)
didn't receive it.
@dohzya
Copy link
Contributor

dohzya commented Feb 20, 2014

The $linkResolver was passed by Document to DocumentLink only, so links nested in a StructuredText didn't receive it. The commit a638e58 fixes that. I hope it fixes your problem :-)

@rudyrigot
Copy link
Contributor

Well, this looks very good to me.

@tremby Do you want to have a look at it before we merge this? Your feedback has been amazingly valuable to us!

@tremby
Copy link
Contributor Author

tremby commented Feb 20, 2014

Everything works as expected with @dohzya's changes, both with the LinkResolver instance and with closures (I will note this in the other ticket too). Please merge his changesets. Thanks very much.

@rudyrigot
Copy link
Contributor

Thanks a lot for your feedback and your time!

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 a pull request may close this issue.

4 participants