I hope this patch was done correctly. If not, let me know and I'll try it again. I'm patching against the original copy of the file/version. I want these to be added to crud, so let me know what I can do to make it easier for you.

CommentFileSizeAuthor
#3 crud_pcorbett.patch12.26 KBpcorbett
crud_0.patch30.2 KBpcorbett

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Active » Needs work

This is a patch, so marking it as such...

In glancing through it, it looks like we have some sort of line-ending problem? The patch seems to delete the entire contents of crud.inc and re-add it. Usually this happens when the file is saved with Windows line-endings instead of Unix ones. To prevent this, open your modified copy of crud.inc in your IDE or advanced text editor (NOT notepad or a simple one like that) and see if they have an option to save it with Unix line endings instead.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

This is a patch, so marking it as such...

In glancing through it, it looks like we have some sort of line-ending problem? The patch seems to delete the entire contents of crud.inc and re-add it. Usually this happens when the file is saved with Windows line-endings instead of Unix ones. To prevent this, open your modified copy of crud.inc in your IDE or advanced text editor (NOT notepad or a simple one like that) and see if they have an option to save it with Unix line endings instead.

pcorbett’s picture

pcorbett commented
StatusFileSize
new crud_pcorbett.patch12.26 KB

Ah, sorry about that. Here's a better one - let me know if it doesn't work - I'm on a Windows machine and used Tortoise SVN to create the diff/patch in unified format.

sime’s picture

sime
any/any
Location Melbourne
commented

I'm happy to help get some of these in one by one. What's the deal with the block code first?

pcorbett’s picture

pcorbett commented

The block code adds a new block and associates roles that have access to that block. It only runs if there is not an existing block with the same name. The update block code updates a block in order to activate/deactivate it.

If this functionality exists in a newer release of CRUD, then it is fine to leave this out. I found it useful in my multisite setup because otherwise I'd have to go into each site manually set a block to appear in a specific region.

Let me know what else you'd like to know about this. Sorry if it's confusing.

sime’s picture

sime
any/any
Location Melbourne
commented

Not confusing, just wanted to get a dialog going and break the patch into chunks. I'll follow up soon.

sime’s picture

sime
any/any
Location Melbourne
commented
Status: Needs work » Postponed (maintainer needs more info)

Not sure how much of the block stuff I'll use. The functions should be modular, so I think adding roles should be separate. I'm going to nicen-up the block functions a little more, at any rate so watch the space.

What is the next area in your patch you'd like to consider?

pcorbett’s picture

pcorbett commented

Block updates look good (and much cleaner than mine). I would say that the ability to add a node would be the next thing. And after that would be the few mssql adaptations.

sime’s picture

sime
any/any
Location Melbourne
commented

Do you mean mysql? You are using MS SQL Server?

Yes, I have a node add function, but I don't want to add it until it is a bit more reliable.

pcorbett’s picture

pcorbett commented

Correct, I'm using mssql. I included some SWITCH / CASE statements in there that accounted for any differences for mssql vs. mysql.

dww’s picture

dww
we/he/they
commented
Component: crud.inc » CRUD functions and includes
Status: Postponed (maintainer needs more info) » Fixed

- install_contact_add_category() is now working properly thanks to #338214: install_contact_add_category() totally broken

- install_taxonomy_add_vocabulary() and install_taxonomy_add_term already exist in both 6.x-1.x-dev and 5.x-2.x-dev.

- install_add_block() is in, and now install_create_custom_block() thanks to #185020: crud.inc logic for inserting to boxes

- I'm not sure what value install_create_node() adds over node_save().

- I'm not using LDAP these days, so I have no way to test that code.

- I just opened #346784: Use db_query_range() instead of LIMIT for DB portability to deal with the various LIMIT related changes you proposed in here. This API shouldn't be trying to act like a DB abstraction layer -- that's what core is for. ;)

From my reading of the patch, that's everything. Therefore, I'm just going to mark this fixed. Monolithic patches (and issues), suck, since they're so much harder to work with. If there's any interest in an install_add_node() or that LDAP code, please open new issues for each distinct feature you want to add so it's easier to review, track, and ultimately commit, the patches.

Thanks,
-Derek

pcorbett’s picture

pcorbett commented

Yeah, some of this has probably been fixed since I posted this over a year ago. Thanks for addressing everything!

The monolith of a patch I posted initially was an error on my part, the second patch I posted should have been a bit more useful. I'll post incremental patches from now on so that you aren't having to deal with multiples at once - makes sense.

Status: Fixed » Closed (fixed)

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

boris mann’s picture

boris mann commented

Since we have ways to split functions into contrib-specific files now, feel free to submit an ldap.inc to go into the contrib folder.

dww’s picture

dww
we/he/they
commented

(And please do so via a separate issue, let's leave this one closed). ;)