Prevent Warning unknown modifier in pathauto.inc by alerting on module conflicts

From the pathauto homepage:

• WYSIWYG Conflict FCKEditor, TinyMCE, etc. If you use a WYSIWYG editor, please disable it for the Pathauto admin page. Failure to do so may cause errors about "preg_replace" problems due to the <p> tag being added to the "strings to replace". See this issue for more details.

I believe that issue will provide you with a solution to your problem.

To be more proactive, though, I'd like to change this to a feature request for the dev version of Pathauto:

We should do a "module_exists('fckeditor') || module_exists('tinymce')" etc. and if any of them exist give a warning on the admin page...

I found a solution by upgrading php on my Hosting from 4.3 to 5.2. That included a necessary library PCRE, which solved the problem. Found it in a post - sorry. I'm new to much of this. Great module once I got everything straight.

Greg

And here's a patch.

It catches all of these editors: 'tinymce', 'fckeditor', 'bueditor', 'htmlarea', 'whizzywig', 'widgeditor', 'wymeditor', 'wysiwyg', 'xstandard', 'yui_editor', 'htmlbox'.

If you have one of those installed it will display a red error box.

It recommends users read the README.txt file.

It then sets a variable so that it will never show the error again.

Thoughts?

Is there a reason the blank line between _pathauto_warn_wysiwygs(); and _pathauto_include();?
There's no need for a double newline before function _pathauto_warn_wysiwygs() { at least.
There's a double space in the warning string.

Also, perhaps something like the following would be nicer than the current array?

Also, since we're setting a system variable, perhaps it would be a good thing to use watchdog() instead of just a drupal_set_message() which will quickly disappear. A watchdog() call will be accessible in the logs and not just disappear once the message has been shown once.

Also also, having the changes to the README.txt in the same patch would be nice, so we know the entirety of what we're reviewing. :)

Thanks for the review. I'll certainly do the cleanups you mention.

I mostly like the watchdog idea. My reason to show this once and not again is that people can configure most WYSIWYG editors to not cause problems with Pathauto. So, there's no need to warn (or watchdog) all the time for something that might not be a problem. Right?

I don't have any changes to README related to this - there's already a note in there about the WYSIWYG conflicts.

I'm also not 100% sure that we need this. I haven't seen any reports of this issue in a long time, but if we add this feature then we could remove the warning from the front page.

We could also add a warning in this system about the category module for the same reasons.

I just meant that drupal_set_message() should be replaced with watchdog() but leave the rest of the logic as it is. This means that the watchdog() call would only happen once, while pathauto_wysiwyg_warned isn't TRUE.

We could also add a warning in this system about the category module for the same reasons.

Restate?

Restated: the category module also has known problems with Pathauto. So, as long as we are going to warn people about module conflicts we should warn about that one too (and add it to README.txt).

At least for me, I tend to ignore the watchdog until I've got a problem. At the point people notice this problem with Pathauto the watchdog message may be deleted from the log. I'd rather give the message at the time the error is introduced and when the user is in a place to do something about it.

Isn't the watchdog message shown? Hohum. As a compromise, perhaps do both? Or perhaps dump the system variable and (conditionally) add the warning "directly" to the affected box? So that it is always shown. (Like we're currently doing when explaining why transliteration isn't enable-able.)

Oooh - now that's an idea I really like!

The field already had a warning about WYSIWYGs which I've removed it in favor of this conditional one.

There's still a double space in the warning text. :) (Between "page." and "Please".)
Also, the $wysiwygs array's last item should be followed by a comma. Also, perhaps the array could/should be sorted alphabetically instead of, eh, randomly?

Looks good otherwise. Not tested.

It was sorted by popularity (more or less, not officially) but alphabetically seems like a better idea for long term maintenance.

Both suggested changes implemented in this patch (hopefully one day I'll start doing one space by habit...).

Just noticed one more string thing! Please the move the space ("t(' You[...]") to before the '<em>'. The space in the translatable string will invariably lead to translations without it - and it is both easier and less error prone in the long run, to simply move the leading space out of the string. :)

Alrighty...I changed to "strong" instead of "em" since we use "strong" on the transliteration info text.

I guess this is close to ready now.

Code looks good to me. Not tested, but it doesn't seem to be dangerous code. If it works for you, do commit. :)

Great, thanks for the nudge. I committed it - http://drupal.org/cvs?commit=131508

Automatically closed -- issue fixed for two weeks with no activity.