[#3414] - Conversation starter for JTable::publish() revision
- Closed
- 3 May 2015
- Medium
- Build: staging
- # 3414
- Diff
- Paladin:JTable/publish
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Not sure how valuable this will turn out to be in the long run, but while I was tooling around the db I noticed that some components used "published" for the publishing state column, and some used "state". Since JTable::publish() is hard-coded for the "published" column, anything using 'state' has to "roll their own" publish method.
By adding a fourth parameter ($column) to JTable's publish method, which defaults to 'published', any of the existing components that rolled their own with 'state' (from appearances, 'state' seems to be the current thinking of what that column should be called) can turn their current publish method into a one-line shim with a call to parent::publish that adds the table column they're using on the end. This results in a savings of around 70 lines per method replaced, if they pulled the entire publish method in.
Change adds one comment line to the docblock, and simply replaces the hard-coded table name with the fourth parameter. Older code is unaffected, because the parameter is written to default to what it is currently hard-coded to be. This simply allows 'state' to be used in place of 'published' for any component that needs it.
Attached to this PR is the change at its very simplest to illustrate the point.
Alternatively, existing components using state could be brought into line with the hardcoded 'published' to achieve the same goal. It just seemed to me that 'state' makes for a more accurate column name and looks to be where we're headed so this makes it possible for everyone to start moving that way now.
I wondered as I wrote it if something in a file like config.xml couldn't trigger this column switch automatically. Didn't see how, but admit I didn't look long before firing this off. Like I said, it's a conversation starter that can be implemented as-is or improved as the conversation runs. If it runs.
I think I prefer a stateColumn property default to published (for BC) and set to state on that tables that use it.
That allows us to set it to null / false if published is not used which will let devs to easily check it in other methods.
+1
I like the idea. Especially since I could reuse the same method for a different purpose as well. In my extension I have for example two such "state" columns, I could just call the parent with two different column values instead of copying the function.
but would it be better to have it as a class variable than a parameter for the method?
but would it be better to have it as a class variable than a parameter for the method?
I'd prefer both 
An argument for the method would allow to reuse the method for multiple columns. In my case I have a "state" column and a "podcast" column. Both basically do the same but for different purposes. Other extensions may have similar things.
A class variable however would allow for very easy useage. You could just specify the "state" column and don't need to override the method at all.
jXtended Finder worked the same way as JTable does now with the hard coded
field names. Building it into 2.5, we added a class var to the adapter to
allow devs to specify without overriding. If this one method is the only
place it's used, adding a param to the method signature is fine. But if
it's to be referenced throughout the JTable inheritance, I'd do a class var.
On Monday, April 7, 2014, Thomas Hunziker notifications@github.com wrote:
but would it be better to have it as a class variable than a parameter for
the method?I'd prefer both [image:
An argument for the method would allow to reuse the method for multiple
columns. In my case I have a "state" column and a "podcast" column. Both
basically do the same but for different purposes. Other extensions may have
similar things.A class variable however would allow for very easy useage. You could just
specify the value and don't need to override the method at all.Reply to this email directly or view it on GitHub#3414 (comment)
.
I mean I'd be interested in doing the same thing to the checkedout/checkedin coloumn names as well :) If it's all classvars one day you can do fun stuff like moving it to a FOF style config.xml file
So I did this as a class variable here #3416. Whilst I understand why @Bakual wants it to be a parameter if you're publishing/unpublishing more than one variable - that to me is where you should be looking to override the method.
Note the config param in the constructor is in anticipation of being able to add similar class variable for checked in and checked out. The JRegistry is because I'm looking at this in future being provided by some kind of XML/YAML/JSON file :)
brianteeman
- | Status | New | ⇒ | Pending |
nicksavov
- | Labels |
Removed:
?
|
||
| Category | ⇒ | Libraries |
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-05-03 20:10:28 |
| Closed_By | ⇒ | zero-24 |