search

PressCrew / infinity-cbox

A fork of the Infinity theme for developing the official CBOX theme
GNU General Public License v2.0
1 stars 1 forks source link

Some options missing when importing #2

Open r-a-y opened 9 years ago

r-a-y commented 9 years ago

Just testing a few things relating to importing the new options during upgrade that are not ported over:

Will update this list as I find more.

MrMaz commented 9 years ago

Thanks for the feedback. Happy to see this being tested!

It's important that the theme folder be named exactly the same as the one you saved the options as. If you originally installed 1.0.x as themes/cbox-theme then you need to name the bleeding edge src exactly the same before activating.

Maybe I should make one exception, and also try to import from "cbox-theme" even if the current theme slug is different. I can explore that if it continues to cause testing headaches.

r-a-y commented 9 years ago

I am symlinking the /infinity-cbox/src/ folder to /cbox-theme/. I do not think this would cause an issue, but I'll try to spin up an older version of cbox-theme and redo the testing later on.

Maybe I should make one exception, and also try to import from "cbox-theme" even if the current theme slug is different. I can explore that if it continues to cause testing headaches.

We could do that as well, but this could be a problem with those that are using .ini files in child themes.

Is it possible to set some defaults if the option does not exist? In the examples I listed above, is "orange" the default button color and is "right" the default sidebar position?

MrMaz commented 9 years ago

The symlink should be fine, as long as WP thinks the theme slug is "cbox-theme" I will do some additional testing to see if I have the same issue. The import stuff was challenging so wouldn't surprise me if there are still bugs. I have it working well, but it's in my sandbox so of course that means nothing really except that is should work, lol.

There are no longer any INI files, all configuration is done programatically... see engine/config/features.php to check out the syntax.

Correct, you are getting the default values for both of those options.

r-a-y commented 9 years ago

The symlink should be fine, as long as WP thinks the theme slug is "cbox-theme" I will do some additional testing to see if I have the same issue.

Forgot to mention if you symlink themes/infinity-cbox/src/ to themes/cbox-theme/, you'll also need to symlink themes/infinity-cbox/src/ to themes/src/.

There are no longer any INI files, all configuration is done programatically...

Oops, sorry! I thought there was going to be some sort of .ini parsing to convert some of the .ini settings over to options, but I was a little daft into thinking that would work since the /cbox-theme/ directory would be wiped out, though the .ini files would not be wiped out in a child theme. Maybe I'm just forgetting how Infinity works :)

Would it be possible to set some custom defaults outside of Infinity core? Or for infinity-cbox, can we just set the desired defaults in engine/config/features.php directly?

MrMaz commented 9 years ago

I already manually (cringe) copied over all of the INI settings to the new syntax. The options importing uses only the database values with a little bit of magic (the part I have been struggling with).

If someone has customized INI files they will need to convert them to the new function calls. If its a major problem I can write a script to convert them. They can't be included in the theme because they throw all kinds of theme check errors.

And actually, you can already override the option settings. Here is an example of how I tweaked some images using an action in the custom dir:

https://github.com/PressCrew/infinity-cbox/blob/master/src/engine/custom/actions.php#L17

MrMaz commented 9 years ago

Btw, the cbox theme is pretty much 99% Infinity core now. If you do a diff between PressCrew/infinity and PressCrew/infinity-cbox you will see what I mean.

I spent quite a bit of time organizing everything so that all of the custom code could live in the engine/custom dir to make pulling down upstream changes as painless as possible.

It might be easier to forget everything you thought you knew about Infinity because it went through just a massive overhaul.

r-a-y commented 9 years ago

And actually, you can already override the option settings. Here is an example of how I tweaked some images using an action in the custom dir:

Nice! I think we'll want to override the default settings to resemble older cbox-theme. I'll open a ticket about this.

MrMaz commented 9 years ago

It's very possible that I made some errors when merging the CBOX styles into Infinity core, so if there is something that doesn't look right, there is a good chance it should be fixed in Infinity... but I can always cherry-pick them from this repo.

(Note: infinity-cbox is a straight fork of infinity, but GitHub doesn't let you call it a fork, something about same account. This is why I moved everything to BitBucket internally and just push copies to GitHub. I didn't make them public to avoid confusion.)

boonebgorges commented 9 years ago

I'm running into some problems (possibly of my own making) with backward compatibility of old settings. It looks to like there's a "rename" chain that kicks off with ICE_Option_Registry::rename_theme_mod(), but it doesn't seem to be happening for me, and I don't see where this function is called in the theme. Do I have to do something manually to init the option "import" or should it happen automatically? (I'm doing a straightforward folder swap - renaming the old cbox-theme and replacing it with the new theme, also named cbox-theme.)

MrMaz commented 9 years ago

Hi Boone...

It only kicks off on theme activation... so you need to activate twenty fifteen or something, then activate cbox-theme again. And actually, this might be what is causing Ray trouble as well

MrMaz commented 9 years ago

I moved the upgrade action to admin_init for now so that the upgrade routines always try to run if the stored version is out of date. This should resolve this issue.

Just a note that the theme version(s) are stored as a theme modification option. The keys are 'ice_upgrade' for Infinity and 'cbox_upgrade' CBOX if you need to wipe them out during testing. Even though CBOX is a fork, it still tracks it's own version independently from Infinity core, so there are effectively two versions stored.

boonebgorges commented 9 years ago

Thanks, @MrMaz. Switching themes seems to have done the trick.

I've done a rough test of the option import and every item I manually checked (maybe 40% of the available options) ported over correctly. So it's looking good :)

Running on 'admin_init' seems good to me. I find plugin/theme activation hooks to be really weird and buggy, so I try to avoid them. 'admin_init' adds just a tiny bit of overhead for your version check.

MrMaz commented 9 years ago

Awesome, that is really good news! I'm going to leave this issue open until @r-a-y is satisfied since it's such a critical piece.