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

Code quality improvements #413

Merged
merged 15 commits into from
Aug 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ This serves two purposes:
- for new features.

### Changed
- for changes in existing functionality.
- Blog posts now have the same opengraph title format as other pages

### Deprecated
- for soon-to-be removed features.
Expand Down
45 changes: 23 additions & 22 deletions packages/framework/src/Models/Metadata/Metadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,42 +76,36 @@ protected function generate(): void
]));
}

if ($this->page->has('canonicalUrl')) {
$this->add(Meta::link('canonical', $this->page->get('canonicalUrl')));
$this->addDynamicPageMetadata($this->page);
}

protected function addDynamicPageMetadata(AbstractPage $page): void
{
if ($page->has('canonicalUrl')) {
$this->add(Meta::link('canonical', $page->get('canonicalUrl')));
}

if ($this->page->has('title')) {
$this->add(Meta::name('twitter:title', $this->page->htmlTitle()));
$this->add(Meta::property('title', $this->page->htmlTitle()));
if ($page->has('title')) {
$this->add(Meta::name('twitter:title', $page->htmlTitle()));
$this->add(Meta::property('title', $page->htmlTitle()));
}

if ($this->page instanceof MarkdownPost) {
$this->addMetadataForMarkdownPost($this->page);
if ($page instanceof MarkdownPost) {
$this->addMetadataForMarkdownPost($page);
}
}

protected function addMetadataForMarkdownPost(MarkdownPost $page): void
{
if ($page->has('description')) {
$this->add(Meta::name('description', $page->get('description')));
}

if ($page->has('author')) {
$this->add(Meta::name('author', $page->get('author')));
}

if ($page->has('category')) {
$this->add(Meta::name('keywords', $page->get('category')));
}
$this->addPostMetadataIfExists($page, 'description');
$this->addPostMetadataIfExists($page, 'author');
$this->addPostMetadataIfExists($page, 'category', 'keywords');
$this->addPostMetadataIfExists($page, 'canonicalUrl', 'url');

if ($page->has('canonicalUrl')) {
$this->add(Meta::property('url', $page->get('canonicalUrl')));
}

if ($page->has('title')) {
$this->add(Meta::property('title', $page->get('title')));
}

if ($page->has('date')) {
$this->add(Meta::property('og:article:published_time', $page->date->datetime));
}
Expand All @@ -123,6 +117,13 @@ protected function addMetadataForMarkdownPost(MarkdownPost $page): void
$this->add(Meta::property('type', 'article'));
}

protected function addPostMetadataIfExists(MarkdownPost $page, string $property, ?string $name = null): void
{
if ($page->has($property)) {
$this->add(Meta::name($name ?? $property, $page->get($property)));
}
}

protected function getPrefixedArray(string $group): array
{
$array = [];
Expand Down
4 changes: 2 additions & 2 deletions packages/framework/src/Services/RssFeedService.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ public function generate(): static
return $this;
}

public function getXML(): string|bool
public function getXML(): string
{
return $this->feed->asXML();
return (string) $this->feed->asXML();
}

protected function addItem(MarkdownPost $post): void
Expand Down
4 changes: 2 additions & 2 deletions packages/framework/src/Services/SitemapService.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ public function generate(): static
return $this;
}

public function getXML(): string|bool
public function getXML(): string
{
$this->xmlElement->addAttribute('processing_time_ms', (string) round((microtime(true) - $this->time_start) * 1000, 2));

return $this->xmlElement->asXML();
return (string) $this->xmlElement->asXML();
}

public function addRoute(RouteContract $route): void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public function test_post_contains_expected_meta_tags()
'<meta name="author" content="Lewis Carroll">',
'<meta name="keywords" content="novels">',
'<meta property="og:type" content="article">',
'<meta property="og:title" content="Adventures in Wonderland">',
'<meta property="og:title" content="HydePHP - Adventures in Wonderland">',
'<meta property="og:article:published_time" content="1865-11-18T18:52:00+00:00">',
]);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/framework/tests/Feature/MetadataTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -394,13 +394,13 @@ public function test_does_not_add_url_property_when_canonical_url_is_null()
public function test_adds_title_property_when_title_is_set_in_post()
{
$page = MarkdownPost::make(matter: ['title' => 'My Title']);
$this->assertPageHasMetadata($page, '<meta property="og:title" content="My Title">');
$this->assertPageHasMetadata($page, '<meta property="og:title" content="HydePHP - My Title">');
}

public function test_does_not_add_title_property_when_title_is_not_set_in_post()
{
$page = new MarkdownPost();
$this->assertPageDoesNotHaveMetadata($page, '<meta property="og:title" content="My Title">');
$this->assertPageDoesNotHaveMetadata($page, '<meta property="og:title"');
}

public function test_adds_published_time_property_when_date_is_set_in_post()
Expand Down
25 changes: 14 additions & 11 deletions packages/framework/tests/Feature/MetadataViewTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,19 @@ protected function assertSee(string $page, string|array $text): string|array

protected function assertAllTagsWereCovered(string $page, array $tags): void
{
$haystack = file_get_contents(Hyde::path("_site/$page.html"));
$links = substr_count($haystack, '<link');
$meta = substr_count($haystack, '<meta');
$expected = file_get_contents(Hyde::path("_site/$page.html"));
$actual = json_encode($tags);

$haystack = json_encode($tags);
$actualLinks = substr_count($haystack, '<link');
$actualMeta = substr_count($haystack, '<meta');
$this->assertEquals(
substr_count($expected, '<meta'),
substr_count($actual, '<meta'),
"Failed asserting that all meta tags were covered in the page '$page'"
);

$this->assertEquals(
$links + $meta,
$actualLinks + $actualMeta,
"Failed asserting that all tags were covered in the page '$page'"
substr_count($expected, '<link'),
substr_count($actual, '<link'),
"Failed asserting that all link tags were covered in the page '$page'"
);
}

Expand Down Expand Up @@ -137,7 +138,8 @@ public function test_metadata_tags_in_empty_markdown_post()
'<link rel="stylesheet" href="../media/app.css">',
'<link rel="canonical" href="http://localhost/posts/test.html">',
'<meta name="twitter:title" content="HydePHP - Test">',
'<meta property="og:title" content="Test">',
'<meta name="url" content="http://localhost/posts/test.html">',
'<meta property="og:title" content="HydePHP - Test">',
'<meta property="og:url" content="http://localhost/posts/test.html">',
'<meta property="og:type" content="article">',
'<meta itemprop="identifier" content="test">',
Expand Down Expand Up @@ -175,7 +177,8 @@ public function test_metadata_tags_in_markdown_post_with_flat_front_matter()
'<meta name="description" content="My description">',
'<meta name="author" content="Mr. Hyde">',
'<meta name="keywords" content="My category">',
'<meta property="og:title" content="My title">',
'<meta name="url" content="http://localhost/posts/test.html">',
'<meta property="og:title" content="HydePHP - My title">',
'<meta property="og:url" content="http://localhost/posts/test.html">',
'<meta property="og:type" content="article">',
'<meta property="og:article:published_time" content="2022-01-01T00:00:00+00:00">',
Expand Down
6 changes: 4 additions & 2 deletions tests/Benchmarks/MarkdownBenchmark.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ class MarkdownBenchmark extends BenchCase
{
/**
* Results history:
* #f4d8d452b 'avg_iteration_time': '0.42493788ms',.
* - #f4d8d452b: 0.42493788ms
* - #fd90ee987: 0.09853745ms.
*/
public function testMarkdownParserFacadeShort()
{
Expand All @@ -19,7 +20,8 @@ public function testMarkdownParserFacadeShort()

/**
* Results history:
* #f4d8d452b 'avg_iteration_time': '2.62538815ms',.
* - #f4d8d452b 2.62538815ms
* - #fd90ee987: 2.07124043ms.
*/
public function testMarkdownParserFacadeFull()
{
Expand Down
4 changes: 4 additions & 0 deletions tests/Benchmarks/PageParserBenchmark.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class PageParserBenchmark extends BenchCase
* Results history:
* - #14c34beb1: 0.17022841ms.
* - #0db04eaef: 0.2702086ms.
* - #fd90ee987: 0.28220031ms.
*/
public function testParseBladePageFile()
{
Expand All @@ -34,6 +35,7 @@ public function testParseBladePageFile()
* Results history:
* - #8e165366a: 0.1986541ms.
* - #0db04eaef: 0.29952221ms.
* - #fd90ee987: 0.30080938ms.
*/
public function testParseMarkdownPageFile()
{
Expand All @@ -53,6 +55,7 @@ public function testParseMarkdownPageFile()
* Results history:
* - #5d044679b: 0.30337441ms.
* - #0db04eaef: 0.39583502ms.
* - #fd90ee987: 0.41671791ms.
*/
public function testParseMarkdownPostFile()
{
Expand All @@ -72,6 +75,7 @@ public function testParseMarkdownPostFile()
* Results history:
* - #6f63f5016: 0.16660199ms.
* - #0db04eaef: 0.2729635ms.
* - #fd90ee987: 0.27649839ms.
*/
public function testParseDocumentationPageFile()
{
Expand Down
14 changes: 8 additions & 6 deletions tests/Benchmarks/SearchIndexBenchmark.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
class SearchIndexBenchmark extends BenchCase
{
/**
* Results history: (with xdebug)
* #5555b676b 'avg_iteration_time': '26.75ms'
* #2d73931ad 'avg_iteration_time': '19.41ms'.
* Results history:
* - #5555b676b: 26.75ms (with xdebug)
* - #2d73931ad: 19.41ms (with xdebug)
* - #fd90ee987: 10.0887394ms.
*/
public function testBuildSearchIndexCommandNoFiles()
{
Expand All @@ -22,9 +23,10 @@ public function testBuildSearchIndexCommandNoFiles()
}

/**
* Results history: (with xdebug)
* #5555b676b 'avg_iteration_time': '120.45ms'
* #2d73931ad 'avg_iteration_time': '105.91ms'.
* Results history:
* - #5555b676b: 120.45ms (with xdebug)
* - #2d73931ad: 105.91ms (with xdebug)
* - #fd90ee987: 36.29438877ms.
*/
public function testBuildSearchIndexCommandWithFiles()
{
Expand Down
3 changes: 2 additions & 1 deletion tests/Benchmarks/StaticSiteBuilderBenchmark.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ class StaticSiteBuilderBenchmark extends BenchCase
/**
* Results history:
* - #49834e374: 76.05051994ms
* - #1cf136fc0: 85.29273033ms.
* - #1cf136fc0: 85.29273033ms
* - #fd90ee987: 49.32703972ms.
*/
public function testParseBladePageFile()
{
Expand Down
6 changes: 6 additions & 0 deletions tests/Browser/HighLevelViewTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ class HighLevelViewTest extends DuskTestCase
{
public $mockConsoleOutput = false;

protected function tearDown(): void
{
parent::tearDown();
sleep(1);
}

public function test_welcome_homepage()
{
$this->browse(function (Browser $browser) {
Expand Down