Image-related tags: some ideas

jsoo · 2017-03-06 11:36:18

Hi all. I’ve been out of the loop for a while; I’m glad to see that the devs are still using their super-powers for the forces of good. Really impressed by how you’re managing the modernisation of the code base while continuing the tradition of easy upgrades and backward compatibility.

Some thoughts about Txp’s stable of image tags.

image_display is a horrible little tag and should be deprecated. It has an un-textpatternish name. It does only one thing (display the image in global image context, i.e., the p query string parameter), takes no attributes, and hard-codes a (gasp, horror) style attribute with the image height and width. See the next item as to how to let image take over the role of image_display.

All of the image output tags introduced in Txp 4.3.0 should have the same behavior regarding image selection. (I think all of them do, except image_author). This behavior should include sensitivity to global image context. Priority would be:

tag id attribute
tag name attribute
$thisimage global (i.e., use within an images tag)
global image context (i.e., the p query string parameter in the URL)

image_index should get the heave-ho too; images can already handle the same function (with some difference in details) and is of course a much more versatile tag.

I’d like to see images default to a form for output, and `image` added as a form type. Admittedly, the hard-coded default output for images is sensible, and not a lot of code, but having a default form in the standard installation would make it easier for users to tweak the output.

I’d be happy to have a go at making these changes if there’s some consensus that these are good ideas.

Bloke · 2017-03-06 13:09:38

Hey, Jeff. Awesome to see you back.

I’d love to see image_index and image_display axed. They suck, and if we need to support the p query param as a fallback for the image tag, then so be it. But I don’t think anybody uses the tags or that URL structure any more, so deprecating them and making everything neater gets my vote.

Image form type: sure. One caveat is that I’ve been trying to find a decent way to allow arbitrary Form types to be created so only the ones in use have a presence in the sidebar. Haven’t found a nice UI flow for it yet.

The fact we have preset, undeletable Forms makes the UI work to support this feature more complex than necessary. If there’s a way to make Txp still easy to use out of the box via the ‘convention over configuration’ mantra, while allowing such default Form content to be deleted with no ill effects to the tag suite or someone’s site (e.g. if the Form is unused??), then I’d love to see it.

By all means, submit PRs to your heart’s content. We’ll roll it in.

Last edited by Bloke (2017-03-06 13:10:30)

jsoo · 2017-03-06 13:34:54

I actually have used the p query param. I don’t care for the messy URL, but global image context is a useful feature. Should it be part of core? Good question.

Another question: what is the view on tag helper functions, such as imageFetchInfo()? There’s a lot of code duplicated across the various image tags. Is it preferred to use such functions to avoid code duplication? I presume taghandlers.php is eventually on the way out in favor of a class for each tag, which I suppose renders my question moot, but for the time being is there a strong preference either way?

Re forms, I’ll just leave that one for now.

Bloke · 2017-03-06 13:47:55

The suite of tag helper functions are useful as you say, but kind of a throwback to the PHP 4 days.

Anything we can leverage to avoid code duplication is a win in my book. How we do that in the object oriented world, I don’t have any clear vision as yet. Helper objects just feel like a hack for adding cheap global functions with a shiny wrapper. Statics: the same. There’s probably a neat OO methodology that’s applicable here, I just don’t know of it. Ideas?

And, yes, longer-term the tags will more than likely be classes, either one class per tag, or perhaps one class per tag suite (which would then allow protected helper methods to be encapsulated in that class itself). Whether grouping tags in that way makes sense, I don’t know. It’s kind of counter to the one-class-one-job paradigm, but does encapsulate related stuff, which is a plus. Presumably we can code tags to implement a set of Interfaces, which might help: ummmm, one for singles, one for containers, one for conditionals, perhaps? Not sure if that gains us anything.

By all means, if you want to blaze the trail here and start moving the image tags over to a class-based structure, it all needs doing in the end. And if it helps reduce code duplication and maintenance overhead, while maintaining performance as far as possible, then I’m all for it.

jsoo · 2017-03-06 14:33:02

Whew, I’ll let someone who has a better idea of overall direction blaze that particular trail.

Stupid, mundane question: just changed txpath on an existing test site to point to Txp’s dev branch, and can’t save Pages or Forms (articles OK). Do I need to do a clean install to get this working?

Bloke · 2017-03-06 14:51:21

Shouldn’t need a clean install, even though table structures might have changed. In theory, logging in should run any necessary upgrade scripts, which’ll resync the database with dev. Do Stylesheets save okay? Anything show up on the admin side when you perform the save if you put the site in debugging mode first?

jsoo · 2017-03-06 15:07:31

No, styles don’t save either. With forms, I get an error message (“Sorry, the form could not be submitted. Please try again later.”). With pages, no error message, but the Save button stays dim + progress indicator. With styles, neither of those things happen, but the changes aren’t saved. Site has been in debugging mode all along. (Changing site prefs works fine.)

Edit: the site was previously running 4.6.2. Have there been changes to the db? /setup/txpsql.php doesn’t show significant changes.

Edit: and I suppose I should have posted this under Feature Ideas; feel free to move.

Last edited by jsoo (2017-03-06 15:35:39)

Bloke · 2017-03-06 15:37:27

jsoo wrote #304456:

With forms, I get an error message (“Sorry, the form could not be submitted. Please try again later.”)

If you open up the browser inspector, anything come back from the Ajax save that might give any clues? Nothing significant from 4.6.2 has changed, aside from those three panels now perform Ajax saving. Presumably your textpattern.js isn’t cached and serving a stale copy?

Edit: no, Stef, if that was the case, the save button would do nothing.

Last edited by Bloke (2017-03-06 15:47:38)

jsoo · 2017-03-06 16:12:56

You were on the right track, though — this is in a multisite setup and I have aliases for various common files, and hadn’t updated all of those. Working now, ta.

michaelkpate · 2017-03-06 18:06:03

Bloke wrote #304449:

I’d love to see image_index and image_display axed.

They don’t seem to be mentioned in any of the early changelogs so apparently they date all the way back to when Textpattern was totally Dean’s vision. I can’t remember when the last time was I used them or imagine why I would.

jsoo · 2017-03-06 18:43:13

So I’m considering altering imageFetchInfo() (from lib/txplib_misc.php). The function was only added in v4.6.0, and the only other functions using it are three of the image tags. I’d rather alter this one than add a new helper function. I could add some extra code to make sure it doesn’t break for any plugin or custom php calling it as currently written, but it seems to me highly unlikely that this is necessary. Sound reasonable?

philwareham · 2017-03-06 19:02:50

Sounds reasonable to me, yes.

Textpattern CMS

Textpattern CMS support forum

#1 2017-03-06 11:36:18