Add initial functionality and tests #1

dlh01 · 2023-12-26T03:45:36Z

This library provides PSR-16 implementations for WordPress projects. It includes adapters for caching data in the object cache, as transients, as options, and as post, term, or other object metadata.

kevinfodness

Awesome work. Left some comments, suggestions, and questions, but nothing blocking. 🍣

kevinfodness · 2023-12-29T16:48:06Z

.github/workflows/code-quality.yml

+    if: github.event.pull_request.draft == false
+    uses: alleyinteractive/.github/.github/workflows/php-code-quality.yml@main
+    with:
+      php: 8.1


I'd recommend we use 8.2 here

kevinfodness · 2023-12-29T16:48:31Z

.github/workflows/coding-standards.yml

+    if: github.event.pull_request.draft == false
+    uses: alleyinteractive/.github/.github/workflows/php-coding-standards.yml@main
+    with:
+      php: 8.1


kevinfodness · 2023-12-29T16:50:14Z

.github/workflows/unit-test.yml

+      - main
+    types: [opened, synchronize, reopened, ready_for_review]
+
+jobs:


This would be a good candidate to test out the strategy defined here: alleyinteractive/create-wordpress-project#63

Otherwise, we would have to mark as required each of the matrix jobs with their versions (4 total combinations).

~~I read that issue and am not sure I understand it. I'll respond over there.~~

Scratch that, I read it again and now understand it!

I tried giving each workflow a generic name in 5c46788, but it just made a mess. GitHub ran all the tests under the specific name defined in the upstream workflow anyway. I'm going to leave the next experiment to someone who knows how GHA works better than I do.

kevinfodness · 2023-12-29T16:51:17Z

README.md

@@ -0,0 +1,55 @@
+# PSR16


Probably want to make this "WP PSR-16" or something to indicate it's for WordPress specifically in the title.

kevinfodness · 2023-12-29T16:55:58Z

composer.json

+  "extra": {
+    "wordpress-autoloader": {
+      "autoload": {
+        "Alley\\": "src/alley/"


Is this specific enough to this library? What about other libraries we maintain that have a namespace of Alley?

AFAIK, this doesn't prevent other libraries from autoloading files with the Alley namespace out of their respective src directories.

kevinfodness · 2023-12-29T16:57:12Z

phpcs.xml

@@ -0,0 +1,49 @@
+<?xml version="1.0"?>
+<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="alleyinteractive/wp-psr16" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/squizlabs/PHP_CodeSniffer/master/phpcs.xsd">


The XSD reference should be updated to the new repo home:

Suggested change

<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="alleyinteractive/wp-psr16" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/squizlabs/PHP_CodeSniffer/master/phpcs.xsd">

<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="alleyinteractive/wp-psr16" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">

ref: https://github.com/Automattic/VIP-Coding-Standards/pull/805/files

kevinfodness · 2023-12-29T17:10:37Z

src/alley/wp/simplecache/class-metadata-adapter.php

+		$out = $default;
+
+		foreach ( $get as $value ) {
+			$out = $value;


Is it possible that getMultiple would ever return more than one value for this key?

Possible, I guess, but only in the case of an implementation error against the spec. I'll update it just in case.

kevinfodness · 2023-12-29T17:12:46Z

src/alley/wp/simplecache/class-metadata-adapter.php

+	 * @return bool True on success and false on failure.
+	 */
+	public function clear(): bool {
+		return false; // Not supported by default in WordPress.


I think this isn't supported by default in memcached but there is a way to do this in Redis. Fine to keep as-is for now but we may want to consider a future enhancement where we could keep an inventory of cache keys that could be deleted on a clear operation (maybe in a separate database table even). That's a common enough problem that we run into where a generic solution would be excellent.

As discussed in Slack, this is "not supported" in the sense that there is no built-in way to delete only the meta keys that have been saved by this library. The library could add support for clear() by adding its own manifest of cache keys that would then be deleted.

kevinfodness · 2023-12-29T17:32:04Z

src/alley/wp/simplecache/class-metadata-adapter.php

+	 * @return iterable A list of key => value pairs. Cache keys that do not exist or are stale will have $default as value.
+	 */
+	public function getMultiple( iterable $keys, mixed $default = null ): iterable {
+		$metadata = get_metadata( $this->type, $this->id );


I may be misunderstanding, but does this library provide functionality for using an object cache implementation for metadata, or does it rely on the underlying WP implementation (e.g., get_metadata here would attempt to pull from WP's cache, which could be a memcached-backed object cache, but this library doesn't care)?

As discussed in Slack, it's more the latter: The takeaway for this adapter is that object metadata itself is used as the cache data store. The fact that core also caches object metadata in the persistent object cache is an implementation detail on the part of core.

Partially reverts commit d6ff981.

This reverts commit 5c46788.

Add initial functionality and tests

3577ee0

dlh01 force-pushed the feature/0.1 branch from 5aca1b1 to 3577ee0 Compare December 26, 2023 03:50

kevinfodness approved these changes Dec 29, 2023

View reviewed changes

dlh01 added 5 commits December 29, 2023 22:35

Use 8.2 in CI

1862add

Update schema location

f7bdf24

Simplify reliance on getMultiple()

d6ff981

Link to PSR

b03b08e

Add "provide" section

ca4ec3f

dlh01 force-pushed the feature/0.1 branch from 96eb6e8 to ca4ec3f Compare December 30, 2023 04:48

dlh01 added 8 commits January 1, 2024 14:03

Expand docs

1cc2aa3

Try using generic names for actions

5c46788

Test against 8.3

6a2d257

Verify key from getMultiple()

cddfcb5

Partially reverts commit d6ff981.

Revert "Try using generic names for actions"

97998d4

This reverts commit 5c46788.

Expand package description

eada637

Keep expanding docs

2f60ba3

Avoid race conditions; report instead that API call was made

eb71cab

dlh01 merged commit 5ff4116 into main Jan 2, 2024
8 checks passed

dlh01 deleted the feature/0.1 branch January 2, 2024 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial functionality and tests #1

Add initial functionality and tests #1

dlh01 commented Dec 26, 2023

kevinfodness left a comment

kevinfodness Dec 29, 2023

kevinfodness Dec 29, 2023

kevinfodness Dec 29, 2023

dlh01 Dec 30, 2023 •

edited

Loading

dlh01 Jan 1, 2024

kevinfodness Dec 29, 2023

kevinfodness Dec 29, 2023

dlh01 Dec 30, 2023

kevinfodness Dec 29, 2023

kevinfodness Dec 29, 2023

dlh01 Dec 30, 2023

kevinfodness Dec 29, 2023

dlh01 Dec 30, 2023

kevinfodness Dec 29, 2023

dlh01 Dec 30, 2023

		@@ -0,0 +1,49 @@
		<?xml version="1.0"?>
		<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="alleyinteractive/wp-psr16" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/squizlabs/PHP_CodeSniffer/master/phpcs.xsd">

Add initial functionality and tests #1

Add initial functionality and tests #1

Conversation

dlh01 commented Dec 26, 2023

kevinfodness left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlh01 Dec 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlh01 Dec 30, 2023 •

edited

Loading