Plugin Review

[tfrommen]

👋😃

As already discussed, I have taken some time to review the current state of Authorship, looking both at the codebase, the functionality and user experience, and developer tooling.

Here's what I've got!

Some of the things I already addressed in a PR. Feel free to take as is, or cherry-pick stuff you like, or close entirely. I thought it made sense to implement some of the changes that are quick for me to do, and just a yes/no decision for you. There's more to do where I didn't want to pose anything, so I left these things completely for you.

User Experience

Input Element

The input element is, by default, just as wide as the content. If it's empty, this means that this is just about 2 pixels, making the cursor not appear as Text tool:

I suggest to always render the input as wide as its container element:

I know that you already have some further UX improvements in mind, including label, border etc., so I'm not going to comment on any of that.

JavaScript Code

Architecture

In my opinion, both the index.tsx and the authors-select.tsx files are doing too much (different things, even).

In the end, the index file will register a plugin that renders the Authorship multi-select input into the PluginPostStatusInfo slot, located in Status & visibility panel. The settings and code that make up that plugin/select would ideally live in one or more dedicated files.

The authors-select.tsx file includes type definitions/interfaces, helper functions, and the actual component, but not the HOC definitions from the index.tsx file. I suggest to split off all type definitions into a new types.ts file, move the helper function into a new utils.ts file or better even individually into utils/arrayMove.ts, and move the complete definition of the Select component, which is currently located in the index file, into the component file. Here, I would extract the anonymous arrow functions passed to both withDispatch and withSelect to allow for better readability and testability (if JS unit tests were ever to get added).

Inside the authors-select.tsx file, both the createOption and the formatOptionLabel functions are independent of anything defined in the AuthorsSelect component/function, hence I suggest to move these functions out of the component. There's no point in redeclaring them on every render.

The SortableMultiValueElement component is independent of anything other than react-select and react-sortable-hoc, meaning only third-party packages. I suggest to move this into a new component file.

And the SortableSelectContainer, as well as the Select component could live in a separate file, too. This would allow for abstracting away some hard-coded props such as class names, formatting, and further configuration, and it would also move the new SortableMultiValueElement component out of the authors-select.tsx file.

Based on previous projects, I suggest the following architecture and file organization, allowing for a better separation of concerns, and thus easier maintainability:

src
+- components
|  +- AuthorsSelect.tsx
|  +- SortableMultiValueElement.tsx
|  +- SortableSelectContainer.tsx
+- utils
|  +- arrayMove.ts
+- index.tsx
+- plugin.tsx
+- style.scss
+- types.ts

👉 I addressed all of the above in #42.

Inline Styles

Why did you decide to use inline styles in the formatOptionLabel function? Could we just add a custom CSS class instead, and define the styles in the style.scss file?

Also, do we really need the additional wrapper around the avatar?

(Not) Using the wp Global

I suggest to not directly reference the wp global, but instead import anything WordPress off the respective @wordpress/... packages/externals.

The two wp.apiFetch references would then be replaced by import apiFetch from '@wordpress/api-fetch';, and wp.plugins.registerPlugin would become import { registerPlugin } from '@wordpress/plugins';. For the latter, we would need to add the @types/wordpress__plugins dependency, as TypeScript doesn't recognize the registerPlugin, for whatever reason. And due to a minor bug in the typings, we now have to pass the icon property in the settings object. It should be marked as optional, but it's not. Adding icon: null is not a big deal, though. 👉 I addressed this in #42.

This also means that we can remove the declare const wp: any; declarations, since wp is not used anymore.

PHP Code

PhpDoc

PhpDoc is "aware" of the current file's scope and imports. So, if you imported (i.e., used) some class, you don't have to provide its FQN. Therefore, you can remove the leading backslash for all WP class such as ´\WP` in parameter or return types. 👉 I addressed this in #40.

Array Diff

In the namespace.php file, there are a few places where you "remove" from a given list of capabilities all values that you defined elsewhere. You are doing this by using array_filter and in_array:

$caps = array_filter( $caps, function( string $cap ) use ( $remove ) : bool {
	return ! in_array( $cap, $remove['delete'], true );
} );

Is there any reason you are not using array_diff?

$caps = array_diff( $caps, $remove['delete'] );

This is much less code, no extra function, and should result in the same list of capabilities...? 👉 I addressed this in #40.

Hook Callback Signatures

Looking at the code, it seems you defined the singatures of action and filter callbacks to be all arguments provided by do_action and apply_filters, respectively.

Personally, I feel this is a bit of unuseful information, especially if you have @param documentation, too. I understand this will save yourself some time if you ever were to need one or more of the currently unused arguments, but it might as wellmean more time spent when creating the function initially. But this is personal preference, I guess. 😉

Use API Functions

Instead of accessing globals directly, I suggest to make use of API functions, where available.

For example, I would use is_author() instead of $wp_query->is_author() (where you first have to "import" the $wp_query global anyway), and get_query_var() instead of $wp_query->get(). This also makes for easier (proper) unit testing. 👉 I addressed this in #40.

Hook Logic

Instead of checking $unsanitized_postarr['tax_input'][ TAXONOMY ] inside the filter callback added to wp_insert_post, I suggest to check it beforehand, and only register the callback if necessary. Or do you expect it to change (which is technically possible, of course, but not what should happen)...?

In the nested filter callback added to views_edit-{$post_type} in the init_taxonomy function, there is some data that is independent of the individual post type, and yet it is computed over and over again. I suggest to move this out to happen before the array_map (orrather: array_walk, see further down) call. Things I am looking at include the user ID, the term object, and maybe even parts of the "mine" link/message. 👉 I addressed this in #40.

Still in the same nested filter callback, if there is no all view (because a plugin removed it), there will also be no mine view as it depends on all to exist. Instead of the more manual foreach-and-compare approach, I would not unset the "mine" filter always, but overwrite it with the new link. And only if there is no term object for the current user, which means there is no post where they are any kind of author, I would remove the "mine" link. 👉 I addressed this in #40.

Possible Errors

In the filter_rest_post_dispatch function in namespace.php, if the author param is set, but not an array, the foreach will break. I'm not sure what the best way to handle this is, but you could either check for array, or cast to array, or something else entirely.

In the init_taxonomy function in namespace.php, you are using array_map, but you are not doing anything with the data. I assume this should be array_walk, or simply foreach instead? 👉 I addressed this in #40.

Same thing in the init_admin_cols function in the admin.php file. I feel this should be a foreach, not requiring a new anonymous function, but could be array_walk as well. Just not array_map. 😉 👉 I addressed this in #40.

Similar to the "mine" view mentioned before, the filter_post_columns function in admin.php, will not add the authorship admin column if the native "author" column has been removed (by some other plugin). If this is what you want, then cool. If we want to render the authorship column always, and just make sure the author one does not render, you would need to adapt the code.

REST Link

I feel the 'https://authorship.hmn.md/{rel}' string, used in filter_rest_response_link_curies could be defined as a constant, just like all the other bits and pieces. 👉 I addressed this in #40.

Taxonomy

The namespace.php file is quite long. How do you feel about splitting of everything directly related to the authorship taxonomy into a new, taxonomy.php file?

Preloading Author Data

Do you think its necessary to preload the author data?

I mean, in theory it's not a bad idea, of course. But there are a few advantages to making an explicit REST API request from within the Block Editor to fetch the author data. The native author select is super slow and I can't think of a reason why users would need to see the UI instantly.

If we'd request the data from the JavaScript app, we wouldn't need to rely on a global variable, which means less manual TypeScript stuff.

But maybe this is what you mean with the TODO not in that function. I wasn't really sure how to understand that comment.

get_author_ids Function

I suggest to introduce a new function, get_author_idsthat would replace thewp_get_post_termsand subsequentarray_map-intvalcalls currently used twice in thetemplate.php` file.

This would make for easier unit testing, and it simplifies both the get_authors and user_is_author function. 👉 I addressed this in #40.

Tests

Just a note that I have not taken a look at the PHP tests, yet.

Tooling

.gitignore

I like the alphabetical sorting. Maybe list folder names before files, though? 👉 I addressed this in #39.

Why is the composer.lock file ignored? The plugin has a Composer production dependency, Asset Loader. Even though the package has an exact version contstraint, I suggest to not ignore the composer.lock file.

Why is the package-lock.json file ignored? The plugin has a few NPM production dependencies, including react-select and react-sortable-hoc. I suggest to not ignore the package-lock.json file.

Composer

Is there anything preventing this from running on PHP 8? If not, I suggest to change the PHP dependency in the composer.json file to "php": ">=7.2".

Just curious, why are you using exact version constraints for some dependencies? Personally, I would default to non-breaking minors (i.e., using caret format, ^1.2.3) and patch-level releases where I'm unsure about the nature of changes in individual releases (e.g., ~1.2.3).

jq

As far as I can see, the node-jq package is only being used to read a property in the Composer config file, ... for a script inside that very file. This package requires quite a bit of overhead—using WSL 2 on Windows, I had to install more than 130 MB of additional build tools to be able to build jq while running npm install. I suggest to remove the node.jq dependency and script, and hard-code the WordPress path in the composer.json file twice. 👉 I addressed this in #39.

NPM

Are you planning on using HM Linter for Authorship? If yes, I suggest to use exact version constraints for the HM Coding Standards, set to the same version that HM Linter would use. For the PHP Coding Standards, you already do this. For both JS and (S)CSS, you use ^1.1.1.

There are some dependencies where I suggest to double-check that they are in the correct class, production vs. development-only dependencies:

• Can the @types/react-select package be a development dependency?
• Should the classnames package be in dependencies?

The lint script, running two commands concurrently, could do with a tiny bit of more information such as the file format (JS and CS, respectively), and maybe a bit of color. 👉 I addressed this in #39.

PHP_CodeSniffer

Consider adding <arg value="s"/> to the config file, instead of specifying -s on the command line (i.e., the test:phpcs script in the composer.json file). This is extra information that shouldn't negatively affect anything anyway, and, currently, both local usage and GitHub Actions already always print Sniff information. 👉 I addressed this in #39.

Do not lint the build/ folder. 👉 I addressed this in #39.

The PSR2R.Namespaces.UseInAlphabeticalOrder sniff has been removed from the PSR2R ruleset. However, the HM Coding Standards are not quite up to date in terms of PSR2R version. I suggest to exclude the sniff for now. 👉 I addressed this in #39.

The PSR2R.Namespaces.UnusedUseStatement sniff does not respect usage in PHP DocBlock or inline comments. Given that IDEs such as PhpStorm and VS Code require use statements for symbols referenced in comments (in order to be able to navigate to the respective file), I suggest to exclude the sniff (and continue using symbols in non-FQN format). 👉 I addressed this in #39.

TypeScript

Also exclude the lib/ folder.

The code is using native ECMAScript modules, not CommonJS. So I suggest to configure TypeScript accordingly, for example, by using "module": "esnext" and "moduleResolution": "node".

Personally, I like to import actual modules and functions, and not namespaces, so instead of this:

import * as React from 'react';

React.useState( /* ... */ );

I would use that:

import React, { useState } from 'react';

useState( /* ... */ );

By default, TypeScript will complain about the React import, but you can add "allowSyntheticDefaultImports": true to the tsconfig.json file.

GitHub Actions

Why not allow fast failures?

What is (still?) failing when using Composer v2? Can we remove the composer:v1 directive for the "Install PHP2 step?

When installing PHP dependencies, we don't care about progress, and we can't handle any interactions, so let's use the --no-progress --no-interaction flags. Also, if you don't want tosee any regular output from Composer, you can use the -q flag, which will then replace both --noprogress and --no-suggest, and it will even strip further output. Errors would still get printed to the console, so you would see them on GitHub. 👉 I addressed this in #39.

Both ESLint and stylelint are not executed on CI. 👉 I addressed this in #41.

PHPUnit

For better coverage information, I suggest to tell PHPUnit where the source code lives. This is done by adding filter.whitelist information. 👉 I addressed this in #39.

The PHPUnit config is missing XML schema information, and the current config is not valid. testsuites.testsuite.directory does not support prefix until PHPUnit 8. I suggest to remove the property, and use exclude for undesired non-test files. 👉 I addressed this in #39.

ESLint

ESLint is incorrectly set up. All JavaScript files are actually TypeScript files, having .ts or .tsx as extensions. By default, ESLint does not lint these files, even with the TypeScript parser and/or plugin configured.

Also, ESLint is outdated: currently v5, while v7 is the most recent version branch. In order for everything to work as expected, ESLint and select dependencies have to get udpated, and the config file, as well as the lint:js script in the package.json file needs updating, too.

---

Great Work! 👏

[johnbillion]

Thanks for the review Thorsten, this was valuable.

The tl;dr answer to most of the "Why?..." questions is that I hadn't gotten around to revisiting a lot of the code since first writing it, and your PRs were very useful in this regard.

I'm going to carry on the with P2 tasks this week, and then go back over the issues you raised that were not fixed in your PRs and open separate issues for those that need addressing.