Make entity_id and entity_table non-required in projects

[larssandergreen]

This makes these fields not required, so projects can be created with New Volunteer Project. This is a follow up on this commit (8 years ago!)

This PR includes the required changes to the XML, plus regenerated sql and DAO, and an upgrade step that will fix this for existing installs (not required for those who already upgraded from 1.x to 2.0, but it won't hurt anything for them).

What needs to happen now is to run civix generate:entity-boilerplate to update the sql and DAO, but before that civix upgrade needs to be run. I tried that, but found that it caused some warnings when installing. I think the process needs to be gone through to check the details mentioned in the upgrade process. I believe volunteer-customdata.xml.tpl will be an issue that needs to be addressed, potentially others, see this message:

[WARNING] If you use the uncommon practice of calling Smarty during installation/upgrade, then you should review/retest
these steps. Determine whether the Smarty include-path is sufficiently up-to-date.

[colemanw]

@larssandergreen maybe the first step is to convert volunteer-customdata.xml.tpl into a mgd.php file.

[larssandergreen]

@colemanw maybe. I got around that by pasting in the old template paths boilerplate, just to see what would happen, and there seemed to be a lot of angular issues after the civix upgrade. Manage Projects wouldn't load at all, for example. Troubleshooting that is not my forte.

Is there a way to regenerate the DAO and sql without doing the civix upgrade?

[colemanw]

Is there a way to regenerate the DAO and sql without doing the civix upgrade?

Maybe by using an old version of civix?

[larssandergreen]

@colemanw Tried 22.02, but same issues. I can try older versions too, but it's weird that there is no civix version in info.xml, so maybe something else is going on here.

[larssandergreen]

@seamuslee001 I see that you regenerated the DAOs for CiviVolunteer a couple years ago. Did you use an older version of civix or do something else? Trying to do the same so that projects can be created, but running into issues with civix upgrades breaking other things.

[colemanw]

@larssandergreen you have to rewind to before the days that civix inserted its version into the info.xml file... circa 2020 I believe.

[larssandergreen]

OK, now working. For reference, I had to use civix v21.04.1, which is available from https://download.civicrm.org/civix/civix.phar-v21.04.1 (though trying to curl it didn't work, downloading it otherwise and then chmod worked).

Some of the angular errors I'm seeing locally (e.g. TypeError: CRM.volunteerApp is undefined) are only local and not anything to do with this PR.

[colemanw]

This looks good to me. The dao updates include some extra metadata but shouldn't be a problem (older versions will just ignore it).

[ginkgomzd]

My concern is that the installer depends on the upgrade routines, so that needs to be refactored to separate the dependency and ensure that what was done in the upgrade routines is fully handled in the new installation process.

I've got family visiting for a couple more weeks so unfortunately can't pitch on this.
Great to have the work you've done!!

[larssandergreen]

@ginkgomzd I'm not sure I follow you completely here. I agree that there is a need to pull the installation steps out of the various upgrade steps they are currently in.

But the reality for this is that the upgrade steps here aren't being triggered on installation (specifically here sql/volunteer_upgrade_2.0.sql is never run on install because the conditional here will not be true on a new install). So this is a start at fixing that so that people can create projects. It doesn't fix everything, but it's less broken out of the box on new installs.

[ginkgomzd]

I think you do follow. It needs to be confirmed that it is less broken. I think it should be confirmed by going through the upgrade steps and assuring they are accounted for in the installer.

[ginkgomzd]

... for me, a known problem with a workaround, is less broken than a sleeping bug.

[larssandergreen]

OK. So just to make it explicit for anyone reading in the future, as I understand it the workaround is that the user should, after installing the extension, go into their database and modify the civicrm_volunteer_project table so that the fields entity_table and entity_id can be null.

[ginkgomzd]

Correct. Thanks for clarifying.
I'm thinking we are close to a comprehensive solution thanks to your work.
I'll be at a computer later this week. Last I reviewed the update steps, they were pretty straightforward. Let me know if you are not seeing a clear path.

[larssandergreen]

Sounds good. Yes, I think there is more to be done similar to this, but I haven't looked in detail.

[ginkgomzd]

Thinking more on this... I don't have concerns about merging the PR, but the issue is scoped too narrowly.
Initially I thought the workaround addressed the problem, but actually, it should be to run the entire 2.0 schema upgrade steps. Doing so on the cli with ignore errors would probably do the trick, or manually running each statement in the file.

There is just too much ambiguity about the problem. It seems like a change in core has made the problem present more often and so it is not clear if no reports of other problems is actually indicative of there being no problems.

Since you have the most context currently, it would be great if you can look carefully at the installer.

[larssandergreen]

Agreed, this is just a first step, it won't solve everything (but I think it can be merged as it improves things).

I think the way forward is just to go through and look at all the upgrader steps (in sql and Upgrade.php) and to make sure those changes are reflected in the XML files. Then regenerate the DAOs and sql with civix as I've done here and everything should be up to date for new installs (and those upgrade steps can be removed from the install function).

And also the options added in volunteer-customdata.xml.tpl could be handled in a mgd file (like this example). This should then allow upgrading to the latest version of civix instead of having to use an old version.

Unfortunately, this is about as much time as I can put into this, so I can't take this on, but I don't think it would be a huge project.

[ginkgomzd]

I'm still on the fence about merging now but will look at everything in detail soon. That's a good checklist and I don't think all of them need to be done to merge and release. I'm also cautious because I think installing from master has become a norm for this extension, so a stable branch is better, even though it is not my favored version control scheme.

I am hesitant about an install that doesn't scream errors but actually has bugs. Being obviously broken has a positive.

[nickperkins]

Being obviously broken has a positive.

Only if it is obviously broken. If you are not a developer, all you see is that there's a bug and you can't create a volunteer project. It's only because I'm a developer that I knew to look at the developer console for the error message.

There is surely a large audience for this extension that aren't using it due to being unable to get past the initial installation steps.

[ginkgomzd]

@nickperkins I don't think you understand me when I speak of the benefit of "obviously broken". It was in contrast to bugs that are not evident. I did not mean that the cause or fix is obvious. "Check engine lights" are annoying, but sometimes they lead to corrective action.

The funding I was expecting and mentioned above did not come through. I believe there is a significant user base but there has been no volunteer contributions of the nature requested, nor are there donations of funding. If you can wrangle any of those, it would move some things along.

Cheers