Plugin Review

# 👋😃

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., `use`d) 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`:

```php
$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`?

```php
$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_ids` that would replace the `wp_get_post_terms` and subsequent `array_map`-`intval` calls currently used twice in the `template.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 `` 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:

```js
import * as React from 'react';

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

I would use that:

```js
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! 👏