Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some options missing when importing #2

Open
r-a-y opened this issue Jun 3, 2015 · 13 comments
Open

Some options missing when importing #2

r-a-y opened this issue Jun 3, 2015 · 13 comments

Comments

@r-a-y
Copy link
Contributor

r-a-y commented Jun 3, 2015

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
Copy link
Member

MrMaz commented Jun 3, 2015

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
Copy link
Contributor Author

r-a-y commented Jun 3, 2015

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
Copy link
Member

MrMaz commented Jun 3, 2015

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
Copy link
Contributor Author

r-a-y commented Jun 3, 2015

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
Copy link
Member

MrMaz commented Jun 3, 2015

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
Copy link
Member

MrMaz commented Jun 3, 2015

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
Copy link
Contributor Author

r-a-y commented Jun 3, 2015

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
Copy link
Member

MrMaz commented Jun 3, 2015

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
Copy link

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
Copy link
Member

MrMaz commented Jun 5, 2015

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 added a commit to PressCrew/infinity that referenced this issue Jun 8, 2015
MrMaz added a commit that referenced this issue Jun 8, 2015
@MrMaz
Copy link
Member

MrMaz commented Jun 8, 2015

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
Copy link

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
Copy link
Member

MrMaz commented Jun 9, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants