WP-API / node-wpapi

An isomorphic JavaScript client for the WordPress REST API
http://wp-api.org/node-wpapi/
MIT License
1.68k stars 191 forks source link

Wordpress 5.5 breaks discoverability #476

Closed acburdine closed 3 years ago

acburdine commented 4 years ago

Ran into this after upgrading one of our wordpress sites to 5.5. This new version of wordpress adds a number of new API routes, including this one: /wp/v2/plugins/(?P<plugin>[^.\\/]+(?:\\/[^.\\/]+)?).

Currently, something in that regular expression is not playing nicely with the discoverability code, because whenever I point wpapi at our wordpress 5.5 site, I get the following error:

SyntaxError: Invalid regular expression: /^[^.\/]+(?:\/[^.\/]+$/: Unterminated group
    at new RegExp (<anonymous>)
    at reduceRouteComponents (./node_modules/wpapi/lib/route-tree.js:86:3)
    at Array.reduce (<anonymous>)
    at reduceRouteTree (./node_modules/wpapi/lib/route-tree.js:185:18)
    at Object.keys.reduce (./node_modules/wpapi/lib/util/object-reduce.js:25:20)
    at Array.reduce (<anonymous>)
    at module.exports (./node_modules/wpapi/lib/util/object-reduce.js:24:3)
    at buildRouteTree (./node_modules/wpapi/lib/route-tree.js:203:9)
    at WPAPI.bootstrap (./node_modules/wpapi/wpapi.js:349:23)
    at new WPAPI (./node_modules/wpapi/wpapi.js:88:4)

Was able to reproduce it with this test:

const wpapi = require('wpapi');

async function test() {
  await wpapi.discover('https://example-wordpress-55-site.com');
}

test().catch(console.error);

I added some logging in that reduceRouteComponents and confirmed that it's the plugins url that's messing things up.

Because discoverability is breaking, it falls back to assuming default routes, meaning none of our custom post types are currently usable via this package 😕

I'll do my best to investigate further and open a PR, but wanted to open this issue in case anyone else runs into the same problem.

acburdine commented 4 years ago

Upon further investigation, it's this regex that's causing the issue, because the plugins regex has nested capture groups.

Not really sure how/if this is easily fixable - it might make more sense to just ignore routes with invalid regexes in the short-term 🤔

msdsk commented 4 years ago

I replaced pattern from that file with const pattern = '\\(\\?(?:P<)(.+?)>(.+?)\\)(?:$|\\/)' - it seems to be matching well for me, but I'm using a very limited functionality so far and haven't tested it any further.

bauerjon commented 4 years ago

Created a temp fix branched off of v1, but maybe works for v2 as well https://github.com/PopularPays/node-wpapi/commit/ac9f187f3df385353af6ed8828eb6c190e7ce1af#diff-0d84322a522b9416af5ea995fa46d971R75-R83

If you're on v1 and want a patch you can reference it directly:

npm uninstall wpapi  && npm install npm install https://github.com/PopularPays/node-wpapi#pp-fix-regex --save
anders-naslund commented 3 years ago

Couldn't download the patch listed above but I made the suggested changes which didn't work for me. Instead I've added these lines which resolved the problem and made autodiscover work with WP 5.5.1 by replacing the regex pattern, seams like a \ was missing. Starting on Line 79 in route-tree.js var myGroupPattern = groupPattern; if (myGroupPattern !== null) { myGroupPattern = myGroupPattern.replace("(?","\\(?"); } // console.log(myGroupPattern); const groupPatternRE = myGroupPattern === '' ? // If groupPattern is an empty string, accept any input without validation /.*/ : // Otherwise, validate against the group pattern or the component string new RegExp( myGroupPattern ? '^' + myGroupPattern + '$' : component, 'i' ); // Only one validate function is maintained for each node, because each node // is defined either by a string literal or by a specific regular expression. currentLevel.validate = input => groupPatternRE.test( input );

NielsdeBlaauw commented 3 years ago

I have created a WordPress core trac ticket https://core.trac.wordpress.org/ticket/51467#ticket. The regex pattern that is in core now is valid PHP, but invalid for JavaScript.

riddla commented 3 years ago

There was a reply in the WordPress core tracker (https://core.trac.wordpress.org/ticket/51467#comment:2):

Testing just the plain pattern [^.\/]+(?:\/[^.\/]+)? and ?[^.\/]+(?:\/[^.\/]+)?). It appears to compile successfully for me in both Safari 14 and Node 12.

It looks like this might be an artifact of how node-wpapi handles transforming the route regex.

This, new RegExp(/^[^.\/]+(?:\/[^.\/]+$/); for instance should actually be new RegExp(/^[^.\/]+(?:\/[^.\/]+)?$/); which does compile successfully. Notice that the )? was dropped.

socrates77 commented 3 years ago

I write this regex \(\?(?:P<|<|')([^>']+)[>']([^\)]*(\))?)\??\) and I tested it ( very lightly with my local wordpress) and it seems to work. if somene would publish a bugfix with that regex maybe after some more tests! Thanks to all

kevinwhoffman commented 3 years ago

I tested today and found that this bug breaks discoverability using the latest wpapi v1.2.1 and WP v5.7.

It looks like @socrates77 addressed this issue in a recent PR https://github.com/WP-API/node-wpapi/pull/479.

Can we help to prepare a patch release so this fix can be published to npm?

kadamwhite commented 3 years ago

wpapi version 1.2.2 has been released, which includes @socrates77' change. Thank you for reporting this, and I am sorry to you all that I was lax in accepting and releasing this. I did adjust the RE slightly while rebasing that change into the v1 branch, so @kevinwhoffman @riddla @NielsdeBlaauw et al, if you still encounter this behavior after updating to v1.2.2, please @ me here with details.

kevinwhoffman commented 3 years ago

@kadamwhite Thanks very much for the release. I successfully tested the following using wpapi 1.2.2 and WordPress 5.7.1:

WPAPI.discover(location.origin).then((response) => console.log(response));
kadamwhite commented 3 years ago

@kevinwhoffman thank you!