Semi-breaking change: change dynamic value tag from % to ##

[k1sul1]

It also requires spaces now.

The regular expression used can be changed from the admin UI.

[luizbills]

Why?

[k1sul1]

Because % is a frequently used character, and has too many edge cases. ## SOMETHING ## is much easier to match.

147

On top of that, I consulted my coworkers in our Slack, and got about 80 responses from more competent people (:trollface:). The takeaway was that in order to take care of "all the edge cases" the expression has to be very complex. One example: (?<validated>\D)(?P<begin>%)(?P<substitute>[\w\-\_]+?)(?P<end>\1)

[rask]

https://regex101.com/r/vdbdke/1

Should strings like foo.bar and fizz-buzz be matchable as \w+ does not match . or - from the looks of it?

[k1sul1]

No. It's not documented (yet) but the supported characters are A-Z, 0-9 & . I guess \w+ could be `[A-Za-z0-9]` (AFAIK they're the same).

Lowercase characters work but I'd stick with ALL_UPPERCASE. You add your dynamic value to the array with a filter, so enforcing it is a bit of a pain.

[luizbills]

I don't know, folks... this is a serious breaking change. I have customers that have more than 20 forms. It would be interesting to have a script that automatically import into the new format.

[k1sul1]

We're going to make breaking changes for the next release anyway. Might as well make them all at once, instead of breaking "userland" every month. I dislike breaking backwards compatibility as much as everyone else, but look at what "never breaking" compatibility has done to WordPress core code.

Migrating to the new format automatically is problematic for the same reasons this change is being made, but I'm taking suggestions.

We could probably submit v2 as a new plugin to wordpress.org and deprecate v1, so no one would accidentally update and break things, but anyone who's ever submitted a plugin knows that it's annoying. If we don't do that, 2.0 is going to display a notice, alerting users of breaking changes.

20 forms may seem like a lot, but converting them to the new format should be a matter of minutes; open each form, copypaste content to a text editor, run search & replace, copypaste changed content back to the form. Rinse and repeat.

[timiwahalahti]

Just a note few things: this plugin has matured a lot since the first version, which was made by @anttiviljami just as a plain-and-simple MVP during a few days at WordCamp. After that many persons have developed this further and added new features, as they have personally needed those. Now, this 2.0 release is streamlining all those features and unfortunately, it will introduce breaking changes.

At the time of release, we need to think how it will be done. Because it needs to be done in a way, that we don't break any live site. But that's a discussion for another issue or PR.

[teppokoivula]

Just an idea – to be honest I'm not at all familiar with the technical implementation here, but could this be a configurable setting for the plugin? Seems like this would not only help avoid breaking changes here, but also help avoid any future issues with the new tag as well.

Regardless, does anyone have a timeframe in mind when this PR might be merged? Got a site that really needs this, so would be happy to hear some kind of rough estimate (or guesstimate) :)

[rask]

@teppokoivula do you mean configurable in wp-config.php (or a hook or something comparable) or in an admin interface? I would vote against adding any more admin interface options than is required.

[k1sul1]

Not a bad idea. I'm not against adding a single select field to an options page if it prevents a breaking change. It's doable but it would have to be a filter. /e is usable in PHP5 so allowing setting a custom value from the admin directly is not smart.

I have this thing called WordCamp Nordic coming up and it's eating a lot of my time, so I can't give an ETA.

You can however use the 1.5 release directly from GitHub. It should work but it may contain bugs that I may have missed, but I tested it pretty thoroughly.

I'm currently using this feature in production with plugin version 1.4.3 & https://patch-diff.githubusercontent.com/raw/libreform/wp-libre-form/pull/117.patch. Using the patch is pretty easy with https://github.com/cweagans/composer-patches.

[timiwahalahti]

Having this as a filterable thing sounds good. I think that just a basic filter would work fine, since you really need to know what you are doing if this value is changed. Having a setting field somewhere in UI makes it too easy to break things.

Like @k1sul1, I have also WordCamp Nordic coming and it eats my time also.

Wouldn't like to promise any ETA for new release as we have worked with it quite a long time already... You are very welcome to make PR about the filter or any other issue that is marked to 2.0 milestone - even a small amount of work helps us to get the new release ready!

[k1sul1]

Without UI we have to make the breaking change.

[You-Honey]

Any ETA for this? @k1sul1 @timiwahalahti

[k1sul1]

ETAs are hard. I'll try and finish this (there's not much to finish in this PR tbh) and some other features before jumping on WPLFB again. I've been busy with other projects that I'm not getting paid for either :+1:

[k1sul1]

I implemented a settings page, it's still a work in progress but you can kinda see where it's going at. So far it includes an option to change the regular expression for this feature, defaulting to the new regex. Then there's an option for disabling [libre-form] shortcode parsing in REST API requests. What else would you like to see? We don't want to include too many options but I think we could do with more than two.

When 2.0 is released, it will include a notice about this changing, but that there's an option to use the old regex. I really don't want to implement "if this forms existed before v2, use old regex", because implementing it would be messy, and we can't ever get rid of it. So few people have used this anyway, as it's not released on w.org.

As a reminder, "converting" the forms to use the new format takes minutes,

When this gets merged with #154, I'll include the admin js globally and combine it into a single bundle, because now we have 3 different admin js files.

[k1sul1]

I'd like some feedback on the implementation. All of our "useful" classes are initiated in WP_Libre_Form, and they're passed the class instance of WP_Libre_Form, giving them access to $wplf->settings->get('x'). This works because the settings class is assigned to WP_Libre_Form->settings.

I don't know what to think about passing the class instance into every class, but it was the only way I got it working without rewriting everything. Calling wplf() (the method exposed with #123) is not possible in the classes constructor, or any method called in the constructor, unless you like fatal errors. The problem? Singletons initing singletons that are still initing.

I don't know why every class of ours is a singleton, none of them have to be. Maybe we could change that as well with 2.0.

"Rewriting" them would also make it possible to drop WPCS and using something like PSR-2. WPCS has forced us to do some very weird indents. Upgrading it to the latest is not possible without also upgrading phpcs and rewriting our config. I don't see point in using WPCS if we're excluding half of the rules.