URL alias reverts to Automatic generate alias when published

Thank you, Jyoti!! The patch in #1 worked for me.

Thanks!

I believe there is an issue with this patch and it should not be in the latest stable release. It is causing unexpected results for those of us using PathAuto in conjunction with the PathAuto Persistent State module because this patch does not respect the settings of PathAuto Persistent State.

When all 3 modules are used together this patch is causing the "Generate automatic URL alias" to become unchecked ALL the time when a node is published. What the original issue reporter should have done was installed the PathAuto Persistent State module to address their issue, not tried changing the way pathauto works within workbench moderation.

In fact doesn't this patch ASSUME pathauto is installed? Some people may not be using pathauto at all so this patch would fail in that case as well.

I see that this patch has been included in workbench 1.4. When I revert this patch out of that version everything goes back to working as I would expect.

Setting priority to major

This patch was included in 1.4, not just dev. Can this actually be fixed for a new release? I could revert it, but it would be better to not disable this functionality if it's already out there.

Anyone have a good idea of what's involved?

So if you want you can check for the existence of pathauto_persist module and/or once that patch is included in pathauto for a certain version of this.

That would keep BC ...

But setting the pathauto to 0 will totally confuse pathauto persist and is not the right fix in the first place.

I'm not sure of the proper procedure. I wish I had seen this while it still existed in dev so it wouldn't have gotten wrapped into an official release. It's definitely an issue because now 1/2 the people are probably experiencing URL problems due to this patch, while the other 1/2 are depending on it to prevent problems. So you are right that it is tricky!

My personal feeling is it should be reverted in dev and then a new release should go out but in a way that would alert users as to what the issue is. But I would like someone with more experience to weigh in on it too.

The point is that this is a general issue in pathauto:

$node = node_load($nid);
$node->title = 'My new title';
node_save($node);

and the setting for the manual alias is broken.

That is what pathauto_persist solves, by storing the state on saving and loading the state again on loading it.

But by setting this state, you overwrite it to 0, which then makes the problem.

Also by setting this you might be setting _all_ to manual aliases, but I have not checked this.

Therefore I would still recommend a revert.

Note that this affects all modules doing any kind of manual saving of entities.

I agree Fabianx! Your solution makes sense. It needs to come out and the idea about making it BC by checking for persist seems like a good move to me.

Is Pathauto Persist the issue? I think my Workbench Moderation 7.x-1.x-dev testing with Pathauto was problem-free. If Pathauto Persist is the issue, would it be better to simply address it in workbench_moderation_store()? I'm initially hesitant to add additional external module exceptions, but aliases are so visible to end users that proper handling is worth it. Besides, there is already Pathauto handling in there.

A bit more extreme: would make sense to create a hook at the end of workbench_moderation_store()? There's a lot of complicated stuff to account for in there, so that would at least give devs a chance to smooth out anything unexpected in a custom module.

As ladybug_3777 pointed out, half of the people are happy with this patch and the other half isn't, so I'd be cautious with a full reversion.

#15: You work around a bug in pathauto that workbench moderation should have no business in ever dealing with.

Else i18n would need to add special code, too, etc.

jbylsma,
No, Path Auto Persist is not the issue. In fact Path Auto Persist FIXES a long standing (4 year old) issue that exists within Path Auto. Essentially those two modules are almost always installed together. Path Auto is working on integrating PA Persist into itself, but until that happens users will be using both modules to make sure that checkbox is correctly checked.

Workbench moderation should have NOTHING to do with either of those modules, at least not in the way this patch has been implemented. This patch essentially forces pathauto's checkbox off no matter what. By doing this it totally confuses Path Auto Persist. This patch also makes an incorrect assumption that pathauto is even installed because it is accessing $live_revision->path['pathauto'] = 0;

The bug in pathauto is being addressed by path auto persist. It should NEVER be addressed here in WB Moderation. That is the main problem we are dealing with here.

Also, it's worth noting, the 1/2 that are happy with this fix are not really using path auto correctly. Technically those people SHOULD have installed path auto persist if they have URL checkbox issues, as stated here: https://www.drupal.org/node/936222

On one of my sites not all my content types are using Moderation. So once Moderation started trying to control what Path Auto did in 1.4 I ended up with a mixed mess on my site and a lot of confusion as to why they acted differently.

Honestly it's a weird situation because the bug in Path Auto has been around SO long.

colan, How would you like to proceed? Do you want us to submit a new patch that reverts this patch but also includes a check for pathauto persist like Fabianx suggests or do you already have that in the works?

@ladybug_3777: What concerned me about this issue was your comments on the duplicate $live_revision->revision = 0; at #2421009: Duplicate $live_revision->revision assignment in workbench_moderation_store(). Looking over this committed patch, I'm not sure why the $live_revision->revision assignment was added; Pathauto doesn't care if its a revision or not. Additionally, the revision assignment was added in another patch, so there shouldn't be any issue removing it here.

Based on FabianX and your comments, I think you are both correct that this needs to be removed. It's a simple patch to rip this out, so I've gone ahead and created one.

To address the original poster's issue, the correct way to deal with this is to use the module PathAuto persistent state. Once the Pathauto patch lands in #936222: Merge in pathauto_persist module functionality to prevent losing manual aliases with node_save() calls, Pathauto persistent state shouldn't be necessary anymore. All thanks to @ladybug_3777 for the info.

Because the current release breaks Pathauto aliasing with moderated content types, it is worth considering creating a new release.

RTBC, looks great. Thanks!

Let's wait a bit to see if there are any more thoughts on this.

Pathauto maintainer chiming in here, this definitely needs to be reverted ASAP to control damage.

I think reverting as quickly as possible is important too. I liked the idea of checking to see if path auto persist is installed to handle backwards compatibility, but honestly after giving it more thought, we probably don't want WB Moderation to even attempt to control Path Auto in anyway even for the sake of BC. Let the BC break if needed because it was wrong.

I vote RTBC too!

@David Reid, don't suppose you have an ETA for when #936222: Merge in pathauto_persist module functionality to prevent losing manual aliases with node_save() calls will be committed? It's been RTBCed to death and its implementation could influence what's done here.

Frankly I have a lot higher priorities in my life right now with our second baby arriving any moment. I'll try to review when I can, that's all I can promise. In the meantime, it's *always* been acceptable to install pathauto_persist for anyone and any site.

Okay, reverted.

Dave, congrats on the new baby and thanks for all the great Drupal work you've given the community.

Thx for the revert colan!

I see this is in dev, any ETA on when it will go into the official stable release?

Title cleanup.

Ignore! sorry