search

Story-Chief / craft-cms-module-storychief-v3

MIT License
2 stars 4 forks source link

Fixed bug for prefix installation #8

Closed balazscsaba2006 closed 2 years ago

balazscsaba2006 commented 2 years ago

There are a few queries that don't take into consideration when Craft is installed with prefixed DB tables. This PR fixes these issues.

gregory-claeyssens commented 2 years ago

Hi @balazscsaba2006

Thank you for submitting this PR and putting in the effort, highly appreciated. Would it be possible to split this into multiple PR's? As you are actually changing a couple of things here, making reviewing and accepting harder.

Without having actually tested anything, my intial feedback:

balazscsaba2006 commented 2 years ago

Hi @balazscsaba2006

Thank you for submitting this PR and putting in the effort, highly appreciated. Would it be possible to split this into multiple PR's? As you are actually changing a couple of things here, making reviewing and accepting harder.

Without having actually tested anything, my intial feedback:

  • The DB prefix addition should already have been applied by Craft automatically (doc). Though I might miss something here.
  • The fix for redactor fields. Instead of using a specific if case to handle redactor fields with the RichTextStoryChiefFieldType it might be a better idea to change the mapping from string manipulation to a mapping array. I think it will improve readability.
  • Adding the anyStatus on update and delete event, this change seems valid (though again, haven't actually tested it yet).
  • Doing a publish when an update event happens and the item wasn't found: I don't think we can accept this change. This would make the plugin behaviour of Craft different to all other integrations. If we would want to do this change we would need to do this accros all our plugins for consistency

@sweet-greg Thank you for the feedback. I'll respond punctually:

  1. The link you sent is valid for v.2. of Craft, and v.3. relies on Yii's Query Builder, so you need the format I added - This was actually fixing a bug we had with the plugin.
  2. It can be improved for sure
  3. 👍
  4. If this is not valid, I understand, then you just need to figure out a way to the scenario when the Craft admin user deletes the content that was initially published through SC. In the event the entry is no longer there, any modification to the content on SC would fail - not at all good. What other options do we have here?
gregory-claeyssens commented 2 years ago

Considering we are on the same page for point 1 to 3, I'll focus solely on issue 4. The behaviour of throwing an error is intentional and the most correct one (imo), I'll try to explain. Firstly, it is intended that all Create, update and delete happens through StoryChief, just to indicate this case is an edge-case; Secondly. If, for some reason, someone would remove an article from the CMS directly, they typically would have a good reason for it. Automatically recreating it on update would probably not be desired;

How can you handle that case in the best possible way:

  1. When updating a removed (or unexisting article) return a descriptive error message (as humanly readable as possible). We typically show these messages to the end user.
  2. The StoryChief user can still unpublish the article in StoryChief. As a 404 would be returned, StoryChief will accept it as a successfull unpublish. They can then proceed to republish it.
gregory-claeyssens commented 2 years ago

Hi @balazscsaba2006 I've just released v1.0.10 which includes most of your changes:

Thank you for pointing these issues out and providing the PR. The released changes are 99% based on your changes

balazscsaba2006 commented 2 years ago

Hi @balazscsaba2006

I've just released v1.0.10 which includes most of your changes:

  • Added support for Redactor fields

  • Fixed the issue with prefixed DB tables

  • Added AnyStatus criteria on update and delete.

Thank you for pointing these issues out and providing the PR.

The released changes are 99% based on your changes

Thank you @sweet-greg! The release looks good, much appreciated