phanan / kupo

✅ Simple site launch checklist checker/validator.
383 stars 23 forks source link

Ideas #12

Open barryvdh opened 7 years ago

barryvdh commented 7 years ago

Nice tool :) I was building something similar in house, but didn't get around to building a nice UI around it.

Some suggestions/ideas that I was thinking of:

Some ideas for checks (besides my PRs)

barryvdh commented 7 years ago

Oh and storing the current URL in the route/hash, so that we can share the url and have the url pre-filled (and started directly when filled)

Probably best with vue-router, but not such an expert in Vue (yet). Edit; see #16

phanan commented 7 years ago

Hi @barryvdh, thanks for the ideas and PRs! I'm not very familiar with PSR-7 TBH, so will need some time to wrap my head around it, but SEO and PageSpeed actually have been planned.

P.S. I'm a huge fan of you and your works, so this is like a "Senpai noticed me" moment ;)

barryvdh commented 7 years ago

PSR-7 is what you are already using, that's the response object from Guzzle :)

What I'm running in to is that you define the constructor and check method in the interface, so you can't easily add extra dependencies to checks. If you would leave the constructor open, but add the crawler etc to the check method, it could be more flexibel.

Also, again with PSR-7, you also get the Response body, so you could think about just passing that. There is also a UriInterface you could use. (And even psr-7 request instead of the URI, as that also contains it, but not needed perhaps)

public function check(Crawler $crawler, ResponseInterface $response, UriInterface $uri);

The rule, with flexible constructor, can be creating using the app container; $rule = $container->make($ruleClassName), which allows for injection of whatever you need, for example the request fetcher.

Currently, the UrlFetcher is a singleton, which keeps the Response as property. But you can't use the UrlFetcher for a different response in a Rule, because then you can't access the old response. You could return the ReponseInterface again, so rules could just use ->getBody() on the response if they need the HTML, but also can access statuscode and headers. And with injecting the Response in the check() method, you don't need to keep a reference of the old response in most cases.

One problem there currently is, is that you manually gzip decode the response. But can't you check the content-encoding header? Guzzle should add the original header as x-encoded-content-encoding, which you could check instead of manually decoding, right? Then you could drop the isGzipped also, and do it properly in your rule based on headers, and just give you this:

/**
     * Fetch an URL.
     *
     * @param UriInterface|string $url
     *
     * @throws \RuntimeException
     *
     * @return ResponseInterface
     */
    public function fetch($url) : ResponseInterface { ..}
barryvdh commented 7 years ago

I can submit a PR for it if you want, but it will break BC (and you already tagged v1..).

Probably not much people using custom rules yet, but still..

phanan commented 7 years ago

Breaking is perfectly fine, we can always tag a v2. Move fast, break things :)

barryvdh commented 7 years ago

Oh and a SSL check (yes/no https) and valid certificate, + warning if < 30 days expiry. Could use https://github.com/spatie/ssl-certificate but that's php7+

phanan commented 7 years ago

Oh and a SSL check (yes/no https) and valid certificate, + warning if < 30 days expiry.

Interestingly I thought about this, too, the other day. The lib looks nice. PHP7 isn't an issue I think – we can force 7 in the next version (we have the breaking changes on the way anyway).

hbakhtiyor commented 7 years ago

also worth to check https://github.com/eyecatchup/SEOstats/