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

complete rewrite #2

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

complete rewrite #2

wants to merge 6 commits into from

Conversation

davidmondok
Copy link
Member

siehe README für Doc. 1-2 Tests hab ich sogar geschrieben :)

Copy link
Member

@wolfgangschaefer wolfgangschaefer left a comment

Choose a reason for hiding this comment

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

Top! Ein paar Änderungen habe ich vorgeschlagen, aber das ganze schaut super aus!


// Import a json containing cache busting hash values, versions, etc.
$scriptsHashFile = new HashFile(get_stylesheet_directory() . '/assets/js/.assets.json');
Copy link
Member

Choose a reason for hiding this comment

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

maybe describe format of assets.json or reference the node package that is creating this
and maybe reference to gulppress

* number is automatically added equal to current installed WordPress version.
* If set to null, no version is added.
*/
public function __construct(string $src, ?array $deps = [], ?string $handle = '', $ver = false)
Copy link
Member

Choose a reason for hiding this comment

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

Alle optionalen params mit = null initialisieren und dann im __construct body $deps ?? [] machen.
Das macht die Benutzung einfacher wenn man nicht die default values setzen (& wissen) muss.

* @param string $key Optional. Lookup key of hash value.
* @return $this
*/
public function addHashFile(HashFile $hashFile, string $key = ''): Asset
Copy link
Member

Choose a reason for hiding this comment

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

wie bei __construct (also ?string $key = null)

*/
public function addHashFile(HashFile $hashFile, string $key = ''): Asset
{
$this->hash = $hashFile->getHashValue($key ?: $this->file->getBaseName());;
Copy link
Member

Choose a reason for hiding this comment

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

und dann wäre hier ein ??

composer.json Outdated
}
],
"require": {
"php": ">=7.2"
Copy link
Member

Choose a reason for hiding this comment

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

in includes/HashFile.php werden json_* funktionen verwenden, also wäre auch ext-json nötig

phpstan.neon Outdated
Comment on lines 15 to 18
- '#^Access to property \$(hide|add|populate) on an unknown class PostTypes\\PostTypes\\Columns\.$#'
- '#^Call to method (hide|add|populate)\(\) on an unknown class PostTypes\\PostTypes\\Columns\.$#'
- '#^Call to an undefined method Carbon_Fields\\#'
- '#carbon_(get|set)_post_meta not found\.$#'
Copy link
Member

Choose a reason for hiding this comment

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

Die ignoreErrors können raus

test/wp-mock.php Outdated
@@ -0,0 +1,75 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

Ganze datei kann gelöscht werden (die referenzen drauf auch)

@davidmondok
Copy link
Member Author

davidmondok commented Jun 19, 2020

Hab dein Feedback noch schnell nachgezogen.

Check failt noch, weil die ganzen WordPress functions nicht erkannt werden.

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.

2 participants