Development
Threads by month
- ----- 2026 -----
- June
- May
- April
- March
- February
- January
- ----- 2025 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2024 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2023 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2022 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2021 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2020 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2019 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2018 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2017 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2016 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2015 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2014 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2013 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2012 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2011 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2010 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2009 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2008 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2007 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2006 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2005 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
September 2005
- 78 participants
- 615 discussions
15 Sep '05
Issue status update for
http://drupal.org/node/28301
Post a follow up:
http://drupal.org/project/comments/add/28301
Project: Drupal
Version: cvs
Component: user.module
Category: feature requests
Priority: normal
Assigned to: jjeff
Reported by: jjeff
Updated by: jjeff
Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/admin.access.hover.help.patch (799 bytes)
Here's a patch that adds a 'title' tag to all of the checkboxes in the
admin/access grid. When you roll over the checkboxes in most browsers a
tool-tip box will pop up with information about what each checkbox is.
Examples:
"anonymous user : edit own blog"
"authenticated user : post comments"
"admin : administer filters"
On a site with many roles and many modules, it becomes very easy to
scroll away from the table headers, so this is essential for figuring
out what you're clicking. It doesn't solve all of the admin/access page
usability problems, but it does make the page a lot more usable.
Little patch. Big help.
-Jeff
jjeff
5
8
Issue status update for
http://drupal.org/node/27364
Post a follow up:
http://drupal.org/project/comments/add/27364
Project: Drupal
Version: cvs
Component: filter.module
Category: tasks
Priority: normal
Assigned to: Bèr Kessels
Reported by: Bèr Kessels
Updated by: m3avrck
Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/filter.module_12.patch (20.31 KB)
Updated patch with Dries fixes in.
m3avrck
Previous comments:
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels
This is a mockup for the Filter UI. Currently it is soo complex, that I
dare to call it broken.
Please comment on this, for I will not ake any patches, when the
general idea is disliked.
The idea is t split filters and input formats better. Filters are
filters, defined in modules. Input formats are bundles of filters. This
is how we have it ATM, but the interface fails to communicate that. I
tried to develop a consistent (with the rest of Drupal) interfce that
makes it clearer what is what.
The menu is as follows:
* administer
** ....
** settings
*** input formats
*** filters
** ...
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB)
* administer
** ....
** settings
*** input formats >> see attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB)
* administer
** ....
** settings
*** input formats >> see attachement 2, tab 2
*** filters
**
Note: The default checkbox lmay seem a bit odd. But checking it will
remove the "default status" from the current "default" one and make
this one "default". The help text, and a drupal_set_message should
explain this.
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB)
* administer
** ....
** settings
*** input formats >> clicking a 'configure' link in the table of
attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB)
* administer
** ....
** settings
*** input formats
*** filters >> see attachement 4
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB)
* administer
** ....
** settings
*** input formats
*** filters >> clicking a 'configure' link in the table of attachement
4
**
------------------------------------------------------------------------
Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels
IMO this makes the interface easier. But other disagree. And because it
also removes one feature: (being able to use E.g. HTML in different
input formats, with different settings)
I'll simply wont fix this :(
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB)
I decided to go for it anyway. Be it in a slightly different way then my
initial mockups. The difference is that I left the filters where they
are, hidden beneath the formats.
So, here is the patch that makes the filter UI more consistent with
screens like blocs, menus, vocabularies et al
also nice to note is that:
93 lines are added, 104 are removed. So netto we have less code :)
(cvs diff filter.module | grep ^+ | wc -l)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB)
Here is how the listing now looks.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB)
And the "configure" screen.
The 'add' screen looks exactly the same, only with the default values
pre-filled.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB)
Oh, And here is a really nice example of why the current screen is no
good.
And here I "only" have four roles. I run a site with 12 roles, imagine
this screen then!
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB)
And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2
codebase. Sorry)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal
I like this very, very much!
This is - indeed - much better than the current UI. This always fits,
and makes the life of people who use a fixed width theme much nicer..
Ber, FYI (you probably know this too), 'admin/access' does also break
fixed width themes due to the fact that it expands the width of the
table when more roles are being added..
Good catch! I like it much more this way..
+1 from me...
------------------------------------------------------------------------
Fri, 29 Jul 2005 07:27:34 +0000 : Dries
The code style needs work (spaces, tabs, 'as' -> 'AS'), the code
comments need work, and debug statements should be removed.
Screenshots look good to me. db_query('UPDATE {filter_formats} SET
cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d',
(int)$cache, $name, $roles, $format); can be written better/shorter
quote-wise.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB)
Dries, thanks a lot for the review. OI fixed your comments. Or so I
hope. code-style.pl was of no use (can that *please* be removed from
core?) because it chokes on the help texts. It also pointed out a lot
of old formatting isues, whic, when fixed wuold flood this patch with
unrelated fixes. I tried to narow my style-searching to the areas I
touched. And I made the comments more verbous.
The query is fixed. it saves no characters, but looks better now.
Thanks for the tip.
The print_r..... blush. thats what I get for not running my default
grep -r print_r on my code after finishing up...
I could not find a lot of spaces and tabs. So it lmight be that i
missed one. I triple checked it though.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:32:21 +0000 : Steven
Moving the role settings inward does fix the wide-table problem, but it
makes the filter configuration much more opaque. We could perhaps fix
this by showing the roles as a comma-separate list in the input formats
table. Such a list can wrap and will not stretch the table.
Other than that:
* Having the instructions for the Roles and Filters form_group() at the
bottom is a bit weird. The distance is too big.
* The table that was used for toggling filters on/off was much more
compact before, and IMO a lot clearer. Why change to a flat list with
the description staggered below each item?
------------------------------------------------------------------------
Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels
consistancy, consistancy and more consistancy.
There really is nothing worse that having a different 'concept' of UI
for every thing in Drupal. We are going in the right direction, with
the blocks being editable objects, menus acting like that, taxonomy,
users, etc..
I wanted to make this act similar. I am not at all happy with the fact
tah the list is still a form. But we need this, for setting the
default.
A big -1 for a comma-separate list. That is extremely hackish, even
worse useabilitywise and just silly
* we should avoid typing when not needed.
* we should avoid errors on input, when they can be avoided (a typo
is made very easy, I know all about it) Form set errors won't fix that
usability -wise.
I agree that you sort-of loose the overview. But IMO that is a minor
loss comared to what we gain, usability-wise.
The wtoo-wide was onlywhat triggered me. But do not think that I am
noly making this patch to fix that issue. This patch is there to
improve usability, through consistancy. o make drupal behave the same
in most places in admin. If you know one screen, you know most of them.
We really should move away from custom-hacks for every page, because
that is useability rule #1: consistancy. Stadardisation, even if
sometimes a custom hack might make things easier in that single case,
it will reduce your overall usability so much that nearly ever it is
worth that hack.
That select-boxes instead of the table issue: consistancy. As well that
it completely broke fluid CSS layouts, my solution is more consistant,
and uses forms the way they are meant. If we cannot use $descritption
in checkboxes they should be removed. but IMO it feels much more
consistant. Tables are for tabular data, and this is simply a list of
items, with additional data.
And i do not really get what you mean with "too far down". Do you mean
that you would like to see the order of the groups different?
------------------------------------------------------------------------
Fri, 29 Jul 2005 13:31:24 +0000 : Dries
I haven't tested Ber's patch yet but looking at the screenshots, it like
what I see. I remember being confused by the current filter UI, and
often, I still am.
------------------------------------------------------------------------
Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman
I dislike the recent movement away from overview pages. I think we can
still provide and overview while allowing editing to happen in a
dedicated form. An example in the wrong direction is 'content types'
admin. It is impossible to get an overview, and the listing page is
worthless. I agree with Steven on this one.
It is true that consistency is desirable. But consistent crap is not.
------------------------------------------------------------------------
Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org
I agree with Moshe. Getting a quick overview about what is set how is
essential for an admin.
------------------------------------------------------------------------
Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques
Overall I like the screenshots. Did not test the patch.
Bèr, what I think Steven (#15) meant by "showing the roles as a
comma-separate list in the input formats table" is that as an overview
page it would be better if you added a list (not an input box!) to
screenshot "input-formats_list1.png" (comment #1). Add a column that
reads "Roles" (or "Enabled for") and then for each row a comma
seperated list of the roles that can use this input format (or maybe
even better a HTML list). You would still need to click "configure" to
_change_ the roles.
The "Default" text (not a radio button) you already show in the
"Options" column is something similar: it immediately shows that this
input format is the default without you having to go to the "configure"
page. Why not do the same for "roles"?
If this is what Steven meant, then I tend to agree with him.
And to help moshe and killes: maybe you could even add a column that
_lists_ the used input filters. Again, only a list, not a way to change
it. That's what the "operations" column is for.
A table like that will "fluid" better than what we have now, still it
is a _complete_ overview of the settings.
I could have misunderstand someone... I have the tendency to do that...
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB)
by popular demand: a comma separated list.
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB)
and a screenshot
------------------------------------------------------------------------
Mon, 22 Aug 2005 18:39:44 +0000 : Dries
Please check your use of print theme('page', $output). Normally, you
just return $output.
------------------------------------------------------------------------
Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels
All instances of theme('page',...) in filter.module are now changed to
return ... ;
furthermore, the patch is the same.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann
Bèr, I think you forgot to attach your updated patch.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB)
my email client has this nice warning system for attachements. Maybe
drupal should search for the word attachement too, and when no att.
found give an error ;). Or maybe i should just learn to pay attention.
Guess that is easier.
Anyway, here it is.
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:22:54 +0000 : Dries
I wanted to apply this patch but:
1. I can't change the default input format. 'Save'-button doesn't
work.
2. I got confused by the fact I can't change the roles of the default
filter. I think the form group description should explain this.
3. form_group(t(filter)) should be form_group(t('filter'), ...).
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck
Found a few more issues:
If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab
at the top and then click 'list filters' link in the text, I get this
warning with PHP5 warning: Missing argument 1 for filter_admin_format()
in \drupal\modules\filter.module on line 378.
Also, it is sort of confusing as to what the 'List' tab implies. For
example, if I click on 'input formats' in the admin menu, I get a list
of the current filters. When I click 'configure' for one of those, I
see the options for that filter. However, it says 'list' in one of the
tab, which doesn't really mean much. I thought that was the list of
filters, not the options for that filter. That should be cleaned up.
Also, there is no way to get back to the full list of filters unless
you click on 'input formats' in the admin menu. A setting/link to fix
this would be great.
Also, where is the link to add filters???
I think the options/layout/tabs should mimic of that of admin > users.
So on admin > input formats, it has the tabs 'list', 'add format' ...
very clear and consistent.
On a configure page for a filter, it should have 'list', 'view',
'configure', 'rearrange' ... this would make it easier to navigate and
get back to the original list of filters.
Make sense? I think this and Dries' comments put this patch over the
top :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 01:48:55 +0000 : tag
In my initial battle to figure out this module's (quite nice)
functionality, what tripped me up the most was actually the
nomenclature.
The way I keep it straight in my head now is to translate "input
format" to "filter set". If you view those (the current) screens with
this in mind, I think it's a lot easier to understand what's what. It
sort of describes this in the help text, but the terms "filter" and
"input format" don't really convey the relationship between the two --
there's no way to intuit that an "input format" is a collection of
"filters". Not to my mind at least... and well, we know nobody reads
instructions...
Are there any technicalities that make renaming "input format" to
"filter set" not accurate? Or does someone have a better term/terms to
better show the relationship of these elements? (I'm aware of filters
in servlet architectures but I don't think there's much of a collision
with that usage).
Anyhow, I guess this doesn't quite address the UI controls and/or
layout, but would still help a lot.
------------------------------------------------------------------------
Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels
I prefer filter set. In fact: I like it a lot. Anyone else ideas on
this?
------------------------------------------------------------------------
Fri, 09 Sep 2005 10:18:18 +0000 : Dries
I too had problems with the terminology; it took me months to get used
to 'input formats' versus 'filters'. Using 'filter sets' might improve
the situation -- especially from an administrator's point of view when
you have to wrap your head around the UI/difference. I'm not native
English, but 'filter set' sounds more explanatory, yet slightly more
geeky so I'm not sure if it flies for users who don't need to
understand the underlying concepts (eg. under the node and comment
submission forms).
------------------------------------------------------------------------
Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels
a heads up: I ma redoing the patch. But decided no to change the name
yet. That is a too big change. IT should be in a separate issue, IMO.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB)
* Bugs as reported by dries fixed.
* The [add] tab not appearing is due to your menu caching, refresh that
please.
* Fixed an additional bug: Some agents do not send TRUE for disabled
checkboxen. Also in current filter admin.
and people: we have a problem. Deleting a filter trows errors, due to
the revisions changes. that happens in filter module now, but also
after this patch. I have too little knowledge of the filter system to
fix that, and IMO that is a separate issue. It should be fixed right
after this patch is committed.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels
FYI: the delete bug is waiting here: http://drupal.org/node/30781
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck
Get this warning in PHP5: Warning: Missing argument 1 for
filter_admin_format() in \modules\filter.module on line 379
I have traced this error back to line 240. Why are there no callback
arguments like above on line 235? Looks like a source of a problem/bug
here so needs to be addressed, not sure exactly how to fix it myself
(otherwise I would). Should be easy it seems.
Also, for a quick and easy usability improvement, on line 239, change
t('list') to t('view') ... makes the menus less confusing.
After those fixes, looks like this patch is ready to be committed :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck
Also, this dialog is a bit confusing:
"Choose which roles may use this filter format
You are editing the default format. For the default format, all roles
must be enabled. Therefore you cannot change them.
"
Maybe make it so that first line doesn't appear there on that page?
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:35:50 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB)
changed that line of help.
Can this please still be taken in consideration before the freeze?
------------------------------------------------------------------------
Tue, 13 Sep 2005 18:29:28 +0000 : m3avrck
Can't apply patch, getting error:
Patch malformed at line 287
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:10:07 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_2_0.patch (14.19 KB)
strange. the dowloaded one breaks too, yet the one i uploaded stll
works.
anyway, rerolled
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:25:25 +0000 : Dries
IMO, the form function needs more work. You need to group the form
construction and the form saving. Right now, when you don't provide a
title, you are given an error and an emptied form. This may be a
left-over from the original code though but it would be cool if you
could tidy this up.
The form_group() description still need a trailing point.
There is a broken link in the following help text on the 'add input
format'-page: /If you notice some filters are causing conflicts in the
output, you can [rearrange] them./.
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:30:12 +0000 : Bèr Kessels
dont have time anymore. sorry.
------------------------------------------------------------------------
Tue, 13 Sep 2005 20:50:34 +0000 : m3avrck
new patch soon!
------------------------------------------------------------------------
Tue, 13 Sep 2005 22:10:25 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/filter.module_10.patch (20.27 KB)
Ok here's a new patch. Lot's of fixes in this one (all of the ones from
above except the one noted below), including support for the new
revisions patch.
Additionally, 'input formats' has been renamed to 'input filters'.
After debate on IRC, and the above comments in this post, it seems that
'input formats' is somewhat misleading and doesn't make sense. 'input
filters' clears this up and makes sense to both native and non-native
English speakers (as tested in IRC anywho ;)). This also makes more
sense since Drupal refers to everything as 'filters' and not 'formats'.
However, there is one issue still left unresolved: If you create a new
input filter, and don't specify a title, page reloads clearing all
filled in fields and prompts the error message. Additionally, a blank
filter is created.
Wanted to get this patch posted, I'll look at it later today but if
someone can get this fixed easily, please do, thanks!
------------------------------------------------------------------------
Tue, 13 Sep 2005 22:38:05 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_3.patch (21.49 KB)
this patch should fix the issue m3 rasies abuot amepty formats being
created.
Yes, i use a simple drupal_goto, in case of a for error. And no, it is
not very easy to get the old posted data back.
------------------------------------------------------------------------
Tue, 13 Sep 2005 23:44:26 +0000 : moshe weitzman
the most visible place for this 'input format' term is on the node form.
thats where ordinary users see this stuff. in that context, i don't
think 'input filters is an improvement over input formats. the question
we are asking our users is 'what is the format of your post'?, not what
filters do you want applied ... just my .02. sorry i wasn't in IRC when
this came up. this need not hold up the whole patch.
------------------------------------------------------------------------
Wed, 14 Sep 2005 06:48:03 +0000 : Bèr Kessels
Moshe. I agreed with you, and was against the name change. In this patch
at least.
However, it slipped in.
Stetting code to be committed for it has gone trough so many review
cycles that this patch has MUCH better code than the rest of the filter
module, which REALLY needs a big overhaul :)
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:37:27 +0000 : Dries
Are you saving the data before validating it? If we want to convert
this form to the new form API, we have to fix this cleanly. Sorry.
I agree with Moshe that 'input filters' is suboptimal. People want to
select how their text is going to be /processed/. The word/filter/
sounds rather technical to me. Also, the difference between a single
filter and a set of filters becomes rather blurry to the point it could
be very confusing.
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:41:42 +0000 : m3avrck
Well I was the one that changed 'input filters' to 'input formats' as
that seemed to be the best thing we could come up. I guess it got
confusing because people wanted this to change but maybe not in this
patch. So I will revert that change in a forthecoming patch.
Additinally, this patch is *not* ready. I was going through the form
logic last night and it is a bit off, hence why errors are displayed
along side a an empty form.
I'm going to work as hard as I can and as fast to clean this patch up
like I mentioned I would yesterday. I'll have some ready soon enough
that will be commit worthy.
------------------------------------------------------------------------
Wed, 14 Sep 2005 14:22:41 +0000 : Bèr Kessels
according to me its a go or leave. if m3 or anyone else wants to take
over fine. I am out. no time.
------------------------------------------------------------------------
Wed, 14 Sep 2005 21:30:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/filter.module_11.patch (28.03 KB)
Ok new patch! Everything is fixed. Code has been reworked and lots of
bugs fixed. Code should also be simpler to read (or at least follow in
terms of logic now). Ready to go!
------------------------------------------------------------------------
Wed, 14 Sep 2005 22:02:26 +0000 : Souvent22
+1. I honestly have not played around with the filter formats ever. I
looked at them, adn they were a mess. Did the patch, and everything was
clear as day. Anything that makes drupal easier to use...AND makes it
easier to understand and make sense should def. be included.
1
0
[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by nedjo 15 Sep '05
by nedjo 15 Sep '05
15 Sep '05
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: nedjo
Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/formcheck_5.js (2.12 KB)
The revised js file.
nedjo
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:06:44 +0000 : Junyor
FWIW, Opera doesn't support the onbeforeunload event. However, this
issue doesn't affect Opera anyway: form contents are retained in
history as long as the page isn't closed. IOW, this feature doesn't
work in Opera, but it isn't needed either.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB)
Updated JS styling and JS-killswitch after talking with Unconed on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck
Just to build on Junyor's comment [6] this functionality is built into
Opera 8 and it *doesn't* produce any errors.
[6] http://drupal.org/#comment-44066
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB)
Updated patch to fix possible problem of overwriting onsubmit event
handler thanks to Thox on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:13:59 +0000 : webchick
I tested this and can confirm that it works in *both* versions of Safari
that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So
looks like this code goes one better over the Blogger.com stuff,
because their stuff doesn't workk in 1.3. Nice job! ;)
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck
Well I'm gonna set this to commit then. Tested and working on IE, FF,
Safari. Doesn't work or break Opera or Konqueror but not needed in
these cases. Thox and Unconded have offered thoughts and I've modified
code as needed. Doesn't seem like there is anything else left except a
commit :)
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels
In general I dislike the feature. Konqueror somehow dies this, and that
is the way it /should/ be. Browsers should take care of this, not the
web app.
But, since it is only konq. this patch has a good enough additional
value :)
I would like to see this patch tested with, tinyMCE and HTMLAREA, at
least. For I am quite sure this will break these modules so bad that
they are near unusable.
Which brings me to the next point: using the textarea hook a simple
module could take care of this.
I would very very much prefer this living in a contrib, or even a core
module. Just not "enforced" on me.
And the last point: if this is for core, please use that textarea hook
too. This is where that hook is for: extending the textareas.
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:32:25 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB)
Updated patch after talking with Thox and Uncloned to attach this event
only to forms with a specific class (hence avoiding the problem of
being on an edit page with other admin/search forms).
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:32:55 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_4.js (1.67 KB)
Updated JS file.
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:45:21 +0000 : m3avrck
Berkes, this patch *does not* break TinyMCE, just tried. However,
TinyMCE does not take into account this script. This should be an easy
patch to TinyMCE.
Also, this doesn't affect just textareas, it affects all fields within
a given form (e.g., text fields, check boxes, etc...). Sure the blunt
of editing/creating a node is in the textarea, but there are still all
of those other changes that can be made (new title, revision status,
front page promotion, etc...) so this needs to account for them all
which it does.
This script is unobtrusive and degrades perfectly well. It is tested
and working in FF, IE, Safari and doens't cause problems in Opera or
Konqueror which don't need this feature anyways (since they already
have it).
The usability boost of this script is too enormous *not* to include in
core. As a contributed module, it is really too flaky, and along those
lines, autocomplete.js, inlineuploads.js and related should be in their
own contributed modules :)
I do see your points and I hope this clears it up. It doesn't cause any
problems and only attaches to specified forms, and is off by default.
Any module can easily make use of this script when they use: form(...,
TRUE). This is the same behavior as the other JS files included with
HEAD as well.
Once this is in core I'll work on a patch to TinyMCE to get that up to
speed ;-) (and as such, TinyMCE still works fine, no probs/errors/etc,
and good reason too if you look at how the code *actually* works ;-)).
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:54:37 +0000 : Bèr Kessels
I see. the "why" for not using form textarea is perfectly valid. And I
tried to explain that, eventough I feel this belongs in the Agent, it
is a usability enhancement for all the others using sillier browsers
;).
And you are right about that part of contributions, exp since you
cannot use hook_texarea, having this in a contrib cannot be achieved.
I hereby retract my hesitations. (though I have no time to reapply the
latest patch and test it on konq. The last JS updates broke drupal on
konq, which Steven fixed, right away though!)
------------------------------------------------------------------------
Thu, 15 Sep 2005 16:31:11 +0000 : m3avrck
Ready to go!
------------------------------------------------------------------------
Thu, 15 Sep 2005 20:50:06 +0000 : nedjo
Attachment: http://drupal.org/files/issues/formcheck.patch (1.37 KB)
Nice work m3avrck. I've made some tweaks to minimize code additions on
the PHP side and improve the js. Mainly, it's now enough to set the
'formcheck' class attribute for a form. function form() will add the
js file, and the needed onsubmit event will be added dynamically to
forms. This is consistent with other core js files, which don't
hard-code event calls but add them dynamically, based on js support.
The useful functionality is unchanged.
1
0
[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by nedjo 15 Sep '05
by nedjo 15 Sep '05
15 Sep '05
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: nedjo
-Status: patch (ready to be committed)
+Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/formcheck.patch (1.37 KB)
Nice work m3avrck. I've made some tweaks to minimize code additions on
the PHP side and improve the js. Mainly, it's now enough to set the
'formcheck' class attribute for a form. function form() will add the
js file, and the needed onsubmit event will be added dynamically to
forms. This is consistent with other core js files, which don't
hard-code event calls but add them dynamically, based on js support.
The useful functionality is unchanged.
nedjo
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:06:44 +0000 : Junyor
FWIW, Opera doesn't support the onbeforeunload event. However, this
issue doesn't affect Opera anyway: form contents are retained in
history as long as the page isn't closed. IOW, this feature doesn't
work in Opera, but it isn't needed either.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB)
Updated JS styling and JS-killswitch after talking with Unconed on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck
Just to build on Junyor's comment [6] this functionality is built into
Opera 8 and it *doesn't* produce any errors.
[6] http://drupal.org/#comment-44066
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB)
Updated patch to fix possible problem of overwriting onsubmit event
handler thanks to Thox on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:13:59 +0000 : webchick
I tested this and can confirm that it works in *both* versions of Safari
that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So
looks like this code goes one better over the Blogger.com stuff,
because their stuff doesn't workk in 1.3. Nice job! ;)
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck
Well I'm gonna set this to commit then. Tested and working on IE, FF,
Safari. Doesn't work or break Opera or Konqueror but not needed in
these cases. Thox and Unconded have offered thoughts and I've modified
code as needed. Doesn't seem like there is anything else left except a
commit :)
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels
In general I dislike the feature. Konqueror somehow dies this, and that
is the way it /should/ be. Browsers should take care of this, not the
web app.
But, since it is only konq. this patch has a good enough additional
value :)
I would like to see this patch tested with, tinyMCE and HTMLAREA, at
least. For I am quite sure this will break these modules so bad that
they are near unusable.
Which brings me to the next point: using the textarea hook a simple
module could take care of this.
I would very very much prefer this living in a contrib, or even a core
module. Just not "enforced" on me.
And the last point: if this is for core, please use that textarea hook
too. This is where that hook is for: extending the textareas.
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:32:25 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB)
Updated patch after talking with Thox and Uncloned to attach this event
only to forms with a specific class (hence avoiding the problem of
being on an edit page with other admin/search forms).
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:32:55 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_4.js (1.67 KB)
Updated JS file.
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:45:21 +0000 : m3avrck
Berkes, this patch *does not* break TinyMCE, just tried. However,
TinyMCE does not take into account this script. This should be an easy
patch to TinyMCE.
Also, this doesn't affect just textareas, it affects all fields within
a given form (e.g., text fields, check boxes, etc...). Sure the blunt
of editing/creating a node is in the textarea, but there are still all
of those other changes that can be made (new title, revision status,
front page promotion, etc...) so this needs to account for them all
which it does.
This script is unobtrusive and degrades perfectly well. It is tested
and working in FF, IE, Safari and doens't cause problems in Opera or
Konqueror which don't need this feature anyways (since they already
have it).
The usability boost of this script is too enormous *not* to include in
core. As a contributed module, it is really too flaky, and along those
lines, autocomplete.js, inlineuploads.js and related should be in their
own contributed modules :)
I do see your points and I hope this clears it up. It doesn't cause any
problems and only attaches to specified forms, and is off by default.
Any module can easily make use of this script when they use: form(...,
TRUE). This is the same behavior as the other JS files included with
HEAD as well.
Once this is in core I'll work on a patch to TinyMCE to get that up to
speed ;-) (and as such, TinyMCE still works fine, no probs/errors/etc,
and good reason too if you look at how the code *actually* works ;-)).
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:54:37 +0000 : Bèr Kessels
I see. the "why" for not using form textarea is perfectly valid. And I
tried to explain that, eventough I feel this belongs in the Agent, it
is a usability enhancement for all the others using sillier browsers
;).
And you are right about that part of contributions, exp since you
cannot use hook_texarea, having this in a contrib cannot be achieved.
I hereby retract my hesitations. (though I have no time to reapply the
latest patch and test it on konq. The last JS updates broke drupal on
konq, which Steven fixed, right away though!)
------------------------------------------------------------------------
Thu, 15 Sep 2005 16:31:11 +0000 : m3avrck
Ready to go!
1
0
Returned Mail: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by drupal-devel@drupal.org 15 Sep '05
by drupal-devel@drupal.org 15 Sep '05
15 Sep '05
The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum.
From: m3avrck <drupal-devel(a)drupal.org>
To: drupal-devel(a)drupal.org
Subject: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
****************************************
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: m3avrck
Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/formcheck_4.js (1.67 KB)
Updated JS file.
m3avrck
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:06:44 +0000 : Junyor
FWIW, Opera doesn't support the onbeforeunload event. However, this
issue doesn't affect Opera anyway: form contents are retained in
history as long as the page isn't closed. IOW, this feature doesn't
work in Opera, but it isn't needed either.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB)
Updated JS styling and JS-killswitch after talking with Unconed on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck
Just to build on Junyor's comment [6] this functionality is built into
Opera 8 and it *doesn't* produce any errors.
[6] http://drupal.org/#comment-44066
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB)
Updated patch to fix possible problem of overwriting onsubmit event
handler thanks to Thox on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:13:59 +0000 : webchick
I tested this and can confirm that it works in *both* versions of Safari
that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So
looks like this code goes one better over the Blogger.com stuff,
because their stuff doesn't workk in 1.3. Nice job! ;)
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck
Well I'm gonna set this to commit then. Tested and working on IE, FF,
Safari. Doesn't work or break Opera or Konqueror but not needed in
these cases. Thox and Unconded have offered thoughts and I've modified
code as needed. Doesn't seem like there is anything else left except a
commit :)
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels
In general I dislike the feature. Konqueror somehow dies this, and that
is the way it /should/ be. Browsers should take care of this, not the
web app.
But, since it is only konq. this patch has a good enough additional
value :)
I would like to see this patch tested with, tinyMCE and HTMLAREA, at
least. For I am quite sure this will break these modules so bad that
they are near unusable.
Which brings me to the next point: using the textarea hook a simple
module could take care of this.
I would very very much prefer this living in a contrib, or even a core
module. Just not "enforced" on me.
And the last point: if this is for core, please use that textarea hook
too. This is where that hook is for: extending the textareas.
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:32:25 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB)
Updated patch after talking with Thox and Uncloned to attach this event
only to forms with a specific class (hence avoiding the problem of
being on an edit page with other admin/search forms).
1
0
Returned Mail: [drupal-devel] [task] Collapsible settings for all settings pages?
by drupal-devel@drupal.org 15 Sep '05
by drupal-devel@drupal.org 15 Sep '05
15 Sep '05
The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum.
From: m3avrck <drupal-devel(a)drupal.org>
To: drupal-devel(a)drupal.org
Subject: [drupal-devel] [task] Collapsible settings for all settings pages?
****************************************
Issue status update for
http://drupal.org/node/30038
Post a follow up:
http://drupal.org/project/comments/add/30038
Project: Drupal
Version: cvs
Component: base system
Category: tasks
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: m3avrck
-Status: active
+Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/drupal_17.patch (9.17 KB)
Rerolled against HEAD.
Since it seems the FORM api is going to go in *after* the freeze, this
should go in now, so before any of the forms are updated to the new API
(so this change is accounted for later instead of having to do another
patch :))
m3avrck
Previous comments:
------------------------------------------------------------------------
Tue, 30 Aug 2005 17:37:22 +0000 : m3avrck
Is there a reason collapsible fieldsets is used *only* on the main
settings page? What about the users setting page? That is really long
and would benefit. And the uploads setting page, as you add more roles,
that page can get really long too.
I think we should add this feature to all standard Drupal settings
pages that are long like the settings page... it really helps and would
make things look more consistent, instead of "why the heck is this on
one page and not them all?".
Need to get this into the next release as well to be consistent with
the approach of this new feature.
------------------------------------------------------------------------
Tue, 30 Aug 2005 20:38:54 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/modules_0.patch (9.17 KB)
I've gone through all of the settings pages for the included default
Drupal modules and have applied the collapsible form fix. This really
does make a *huge* difference... it makes the Settings interface more
consistent and greatly improves readability. Let's get this one in! :)
------------------------------------------------------------------------
Tue, 30 Aug 2005 21:32:11 +0000 : Souvent22
Installed, and used the patched. Worked great. Much needed. :). +1
------------------------------------------------------------------------
Tue, 30 Aug 2005 21:54:42 +0000 : chx
-1 and +1 at the same time. +1 but only after the forms API is in --
it'd be better if we won't break the fastly evolving patch.
------------------------------------------------------------------------
Tue, 30 Aug 2005 22:28:25 +0000 : m3avrck
Beat me to the post :) I just was reviewing the Forms API patch and
realized this one is no longer applicable if that goes through.
chx, can you include the fixes in this patch with the forms api? it
just makes a few extra menus 'collapsible' which with the new API,
requires even less code change, haha!
1
0
15 Sep '05
The following message from killes <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum.
From: killes <drupal-devel(a)drupal.org>
To: drupal-devel(a)drupal.org
Subject: [drupal-devel] [task] Extend db_query()
****************************************
Issue status update for
http://drupal.org/node/17656
Post a follow up:
http://drupal.org/project/comments/add/17656
Project: Drupal
Version: cvs
Component: database system
Category: tasks
Priority: normal
Assigned to: killes(a)www.drop.org
Reported by: killes(a)www.drop.org
Updated by: killes(a)www.drop.org
Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/db_query_3.patch (2.85 KB)
Kjartan prefers this version. I also added some comment.
killes(a)www.drop.org
Previous comments:
------------------------------------------------------------------------
Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db-query.patch (2.16 KB)
We should make our database abstraction layer more robust and ensure
that module authors can use it without string manipulations inside the
query. Several queries use implode() to get their arguments into the
query. This is undesirable as we rely on the module author to check the
keys and values of such arrays for exploitation attempts.
I have created the attached patch which shouldbe able to allow us to
not use implode anymore.
A minor problem is that all inserted values will be treated as strings.
This might be a problem with PostgreSQL at least. However, the same
strategy is already used in Drupal core without any complaints I know
of.
Summary: This patch will alow us to simplify some code in node.module,
user.module, taxonomy.module and probably others.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:03:58 +0000 : killes(a)www.drop.org
It's a patch.
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db-query_0.patch (2.07 KB)
Squeezed out two lines of code after consultation with Karoly. Adds only
10 loc (plus some docs).
------------------------------------------------------------------------
Mon, 21 Feb 2005 17:23:08 +0000 : chx
Do I need to say +1?
------------------------------------------------------------------------
Thu, 03 Mar 2005 00:15:10 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db-query_1.patch (2.69 KB)
After some discussion with Adrian at Drupal Con we found out that we do
not know why node_save currently works with pgsql. It currently assumes
that all db columns are strings. It seems to work but we should not rely
on it.
Here is a patch that checks for the type of field that is inserted.
It needs testing.
------------------------------------------------------------------------
Tue, 26 Jul 2005 01:17:04 +0000 : drumm
+1 for making this into an API. I've seen too many hacked together query
builders in Drupal and Contrib. I have not tested.
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:30:29 +0000 : Bèr Kessels
untested. a big +1 for the feature
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:41:52 +0000 : killes(a)www.drop.org
the patch still applies. the new patch here updates node_save to use it.
Untested.
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:42:24 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/node-%a.patch (926 bytes)
the patch still applies. the new patch here updates node_save to use it.
Untested.
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:45:17 +0000 : Cvbge
"After some discussion with Adrian at Drupal Con we found out that we do
not know why node_save currently works with pgsql.
"
Well, it does not work. Real life exampless of not-working include
flexinode (my experience) and forum module (as someone reported).
Probably also others.
The bug occurs when a normal user (but with necessarily rights) adds a
node and he has no controls (the 'moderated', 'sticky', 'published'
etc). The, in node_load() $node->sticky, $node->moderated (and maybe
others) are set to FALSE (or TRUE, but in this case it works).
When doing printf("%s", FALSE) the FALSE is change to empty string. The
sticky and moderated db fields are numeric and postgresql do not accept
'' (empty string) as a value of integer type.
The result is for example such error:
warning: pg_query(): Query failed: ERROR: invalid input syntax for
integer: "" in ..../includes/database.pgsql.inc on line 45.
user error:
query: INSERT INTO node (title, uid, type, teaser, status, moderate,
promote, sticky, body, comment, created, changed, nid) VALUES('xx',
'2', 'flexinode-1', '<div class="flexinode-body flexinode-1"><div
class="flexinode-image-3"><div class="form-item">
<label>Zdjęcie:</label><br />
<img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [1]
</div>
</div></div>', '1', '', '1', '', '<div class="flexinode-body
flexinode-1"><div class="flexinode-image-3"><div class="form-item">
<label>Zdjęcie:</label><br />
<img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [2]
</div>
< in ..../includes/database.pgsql.inc on line 62.
[1] http://drupal.org/..../pliki//tmp/male.jpg
[2] http://drupal.org/..../pliki//tmp/male.jpg
------------------------------------------------------------------------
Wed, 27 Jul 2005 13:03:14 +0000 : Cvbge
Attachment: http://drupal.org/files/issues/node.module_5.diff (500 bytes)
Here's a quick fix for 4.6
------------------------------------------------------------------------
Wed, 27 Jul 2005 13:10:54 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/node-a.patch (1.8 KB)
the last patch wasn't good.
------------------------------------------------------------------------
Mon, 08 Aug 2005 08:19:14 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db_api_taxonomy.patch (3.5 KB)
Here is an additional patch for taxonomy.module
it removes the two custom functions that are used for inserts and
updates.
Needs testing.
@Cvbge: shoduld we have a better test than just is_numeric in
_db_argument_type to fix the pgsql problems? We could also try to pass
$value by reference and cast it to int. this function is only used for
updates/inserts so we can afford an additional check.
------------------------------------------------------------------------
Mon, 08 Aug 2005 08:37:25 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db_query.patch (2.74 KB)
Here's a revised version fo the original patch. _db_argument_type now
gets the $value argument by reference and we check for bools too.
numerics and bools are cast to int.
------------------------------------------------------------------------
Mon, 08 Aug 2005 10:34:36 +0000 : Bèr Kessels
I cannot say anything about performance for I do not know how to
benchmark this. But I tried it, And rewrote some queries in a custom
module, to use it, and like it a lot.
The code becomes cleaner, and better readable. But above all, i see
this idea going into a very interesting and good direction: that of
'more' query builders in core'.
a +1!
------------------------------------------------------------------------
Mon, 08 Aug 2005 10:51:14 +0000 : Cvbge
I'll be talking about postgres db only.
The patch won't work as expected in some cases.
Let's say we have a text/char/other character column and we want to
insert a text that looks like a number. The _db_argument_type() will
recognize the value as a numeric/number etc and will use $key = %d
format. Even not considering printf() numeric conversion this can lead
to incorrect value inserted into db.
Examples of such problematic texts are: 01 (leading 0), 2e2 (scientific
notation), probably also text with trailing/leading white spaces. When
inserting such data into db without quotes the postgres db will think
this is a number and will change 010 to 10, 2e2 to 200 . Here is an
example:
piotr=> CREATE TABLE t(c CHAR(10));
piotr=> INSERT INTO t VALUES (01);
piotr=> INSERT INTO t VALUES (2e2);
piotr=> INSERT INTO t VALUES (-01);
piotr=> INSERT INTO t VALUES (000);
piotr=> SELECT * from t;
c
------------
1
200
-1
0
(4 rows)
I think it's safe to use '%s' everywhere. I could not find it writtent
directly in postgres documentation (although I remember reading it
somewhere some time ago). Postgres will convert the string to correct
integer/numeric/bool value.
------------------------------------------------------------------------
Sat, 13 Aug 2005 01:14:41 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db_query_0.patch (2.69 KB)
Ok, the patch now does only distinguish between integerss and the rest.
This is needed to decide whether we need quotes.
------------------------------------------------------------------------
Sat, 13 Aug 2005 09:40:59 +0000 : Cvbge
I'm sorry, but I see no reason for this integer check. Can you explain
why there is a need to differentiate between integers and other types?
The only problem I see is with converting bool(FALSE). This value is
converted to empty string ('') and when trying to insert it into
numeric, float, bool, date (probably any column that is not text type)
etc you get error.
One solution would be to check for bool(FALSE) and convert it to 0.
This would work for numeric-like and bool fields. But it would not work
for DATA columns (would produce error). Also, entering '0' into text
column might not be what the author wanted.
But this is more a problem with the coder - if you have integer/date
etc field, insert integer/date/etc data type, not bool!
Side note: the integer-version, the one without '', will be used only
for integers (i.e. ..., -2, -1, 0, 1, 2, ... see
http://php.net/manual/en/language.types.integer.php)
------------------------------------------------------------------------
Sat, 13 Aug 2005 09:43:19 +0000 : Cvbge
"DATA" should be "DATE"
------------------------------------------------------------------------
Sun, 14 Aug 2005 00:53:34 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db_query_1.patch (2.71 KB)
Ok, another attempt. There is indeed no real reason to use %d for
integers, they work fine as strings and are converted by sprintf to
strings independend of %d or %s. But it is the Right Thing(tm), so I
kept it.
Booleans, however, need the %d formatter in order to be converted to 0
and 1. TRUE to 1 always works, but FALSE to 0 only works with %d.
All tests on php 4.3.x.
------------------------------------------------------------------------
Thu, 18 Aug 2005 21:46:04 +0000 : Cvbge
The part I was objecting previously looks ok now. I haven't tested the
code though.
I still think it'd be nice to change php NULL to real SQL NULLs, but I
don't have how to do it (at least not with current approach). The best
would be if there was (s)printf flag that would just ignore it's
argument...
------------------------------------------------------------------------
Sat, 27 Aug 2005 19:36:04 +0000 : Thomas Ilsche
Attachment: http://drupal.org/files/issues/node_save.patch (907 bytes)
+1, tested
Runs nicely but maybe NULL and floats might need special treatment.
I have attached an INCOMPLETE patch for node_save using the new feature
that also fixes the following issue.
------------------------------------------------------------------------
Tue, 30 Aug 2005 18:13:48 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/node_save_0.patch (3.65 KB)
Here is an updated patch that takes a lot of the code I added to
node_save out again (approx 20 loc). it also fixes a docs glitch in
that function. Needs testing and is to be used with the patch from
http://drupal.org/node/17656#comment-39222
------------------------------------------------------------------------
Sun, 04 Sep 2005 18:13:36 +0000 : Cvbge
A remark about NULL values:
in this patch they are treated as integers and thus allways changed to
'0'.
In existing code the '' was INSERTed into text-like columns (%s) if
didn't check if the variable was set (or did not care).
If they now use %a and still do not check for NULLs the '0' will be
inserted which migh or might not create problems (it won't matter for
if()s, but maybe for other uses?)
------------------------------------------------------------------------
Sun, 04 Sep 2005 19:50:58 +0000 : killes(a)www.drop.org
Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core.
------------------------------------------------------------------------
Sun, 04 Sep 2005 20:56:41 +0000 : Cvbge
You're probably right, then
1. not checking if a variable is set is a bug (and I've already filled
a bug for one of such bugs, can't find it - sticky, moderated etc.
fields were not set at all when not selected when submitting a post)
2. There's already a lot of DEFAULT NULL in the database schema...
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:03:09 +0000 : Cvbge
Hello, NULLs again.
Previously I wrote that NULLs are treated as integers and passed with
%d and converted to 0.
This is of course not true [were I sick when writting it?]. They are
treated as strings and converted to '' (empty string)
This will create similar problems as FALSE. Maybe NULL should be
treated as %d (and converted to 0) after all?
------------------------------------------------------------------------
Tue, 13 Sep 2005 16:06:30 +0000 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/db_query_2.patch (2.72 KB)
Ok, I've included is_null() in the condition.
------------------------------------------------------------------------
Tue, 13 Sep 2005 16:31:55 +0000 : Cvbge
ok, last thing (really ;)): maybe add some information to docs saying
that null is converted to '0' (not to '') ? :)
1
0
Returned Mail: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by drupal-devel@drupal.org 15 Sep '05
by drupal-devel@drupal.org 15 Sep '05
15 Sep '05
The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum.
From: m3avrck <drupal-devel(a)drupal.org>
To: drupal-devel(a)drupal.org
Subject: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
****************************************
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: m3avrck
Status: patch (ready to be committed)
Attachment: http://drupal.org/files/issues/drupal_18.patch (2.23 KB)
Updated patch after talking with Thox and Uncloned to attach this event
only to forms with a specific class (hence avoiding the problem of
being on an edit page with other admin/search forms).
m3avrck
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:06:44 +0000 : Junyor
FWIW, Opera doesn't support the onbeforeunload event. However, this
issue doesn't affect Opera anyway: form contents are retained in
history as long as the page isn't closed. IOW, this feature doesn't
work in Opera, but it isn't needed either.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB)
Updated JS styling and JS-killswitch after talking with Unconed on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck
Just to build on Junyor's comment [6] this functionality is built into
Opera 8 and it *doesn't* produce any errors.
[6] http://drupal.org/#comment-44066
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB)
Updated patch to fix possible problem of overwriting onsubmit event
handler thanks to Thox on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:13:59 +0000 : webchick
I tested this and can confirm that it works in *both* versions of Safari
that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So
looks like this code goes one better over the Blogger.com stuff,
because their stuff doesn't workk in 1.3. Nice job! ;)
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck
Well I'm gonna set this to commit then. Tested and working on IE, FF,
Safari. Doesn't work or break Opera or Konqueror but not needed in
these cases. Thox and Unconded have offered thoughts and I've modified
code as needed. Doesn't seem like there is anything else left except a
commit :)
------------------------------------------------------------------------
Tue, 13 Sep 2005 17:13:48 +0000 : Bèr Kessels
In general I dislike the feature. Konqueror somehow dies this, and that
is the way it /should/ be. Browsers should take care of this, not the
web app.
But, since it is only konq. this patch has a good enough additional
value :)
I would like to see this patch tested with, tinyMCE and HTMLAREA, at
least. For I am quite sure this will break these modules so bad that
they are near unusable.
Which brings me to the next point: using the textarea hook a simple
module could take care of this.
I would very very much prefer this living in a contrib, or even a core
module. Just not "enforced" on me.
And the last point: if this is for core, please use that textarea hook
too. This is where that hook is for: extending the textareas.
1
0
15 Sep '05
The following message from m3avrck <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum.
From: m3avrck <drupal-devel(a)drupal.org>
To: drupal-devel(a)drupal.org
Subject: [drupal-devel] [task] New drupal forms api.
****************************************
Issue status update for
http://drupal.org/node/29465
Post a follow up:
http://drupal.org/project/comments/add/29465
Project: Drupal
Version: cvs
Component: base system
Category: tasks
Priority: critical
Assigned to: adrian
Reported by: adrian
Updated by: m3avrck
Status: patch (code needs work)
I agree with Ber and Souvent. I think we should all focus on our pet
patches for the Drupal 4.7 freeze (which I think should be Friday give
us that one day ;). After that, we should have a "sub-freeze" where
*only* FORM related API patches go in, set this for 3-4 days after the
4.7 freeze. Then, once that is complete, *then* get back into the
regular bug/performance/usability freeze over all.
m3avrck
Previous comments:
------------------------------------------------------------------------
Tue, 23 Aug 2005 11:34:00 +0000 : adrian
Attachment: http://drupal.org/files/issues/form.inc (20.53 KB)
This is the first check in of the new forms api code.
The system has been designed to co-exist with the current forms api,
and is contained in a new
include file (includes/form.inc).
Forms are now defined in their component arrays, similar to how menu
items are defined.
example :
<?php
$form['body'] = array(type => 'textarea', default_value => $node->body, cols => 60, rows => 60);
?>
Elements can also be nested, and the $edit follows this definition. For
instance :
<?php
$form['author'] = array(type => 'fieldset', title => t('Authoring information'), collapsible => TRUE, collapsed => TRUE, weight => -1);
$form['author']['name'] = array(type => 'textfield', title => t('Authored by'), maxlength => 60,
autocomplete_path => 'user/autocomplete', default_value => $node->name, weight => -1);
?>
All the properties used are defined as constants, and are documented
for each of the elements, and individually.
------------------------------------------------------------------------
Tue, 23 Aug 2005 11:46:19 +0000 : adrian
A patch for node.module, blog.module and taxonomy.module that changes
them to use the new form format. This patch is very far from complete,
but I wanted to get the code out so that i'm not working alone anymore.
------------------------------------------------------------------------
Tue, 23 Aug 2005 12:08:01 +0000 : adrian
Attachment: http://drupal.org/files/issues/forms.patch (9.98 KB)
The actual patch =)
I forgot to mention, this adds a new hook .. namely hook_elements,
which allows us to define the defaults for the elements (ie : cols and
rows for textareas) meaning they don't have to be defined for each of
the elements.
------------------------------------------------------------------------
Tue, 23 Aug 2005 12:09:11 +0000 : chx
A few notes from my conversation with adrian. valid => array('integer',
'uid') for this to work you need function valid_integer($element) and
valid_uid($element). $extra for form_select is legacy and really
needed.
------------------------------------------------------------------------
Tue, 23 Aug 2005 12:39:13 +0000 : fago
i really like this approach.
further i'd like to see the possibility to define an additional class
to a form element, which is currently not working. so we 'd have to
bring _form_get_class() and drupal_attributes() together.
------------------------------------------------------------------------
Tue, 23 Aug 2005 12:56:31 +0000 : adrian
that works already.
<?php
$form[attributes]['class'] = 'someclass';
?>
Although I am considering just adding a class property ...
ie:
<?php
$form[class][] = 'someclass';
?>
The fact that this is done via arrays, it means that the developer can
add classes as he or she sees fit.
------------------------------------------------------------------------
Tue, 23 Aug 2005 13:29:10 +0000 : fago
really?
i don't think so.
e.g.
$checkbox = '<input type="checkbox" class="'.
_form_get_class('form-checkbox', $element[required],
_form_get_error($element[name])) .'" name="'. $element[name] .'" id="'.
$element[id].'" value="'. $element[return_value] .'"'. ($element[value]
? ' checked="checked"' : '') . drupal_attributes($element[attributes])
.' />'
so we will end up with two class attributes, which won't work and isn't
standard compliance.
your css property idea would be ideal imho.
------------------------------------------------------------------------
Tue, 23 Aug 2005 13:40:12 +0000 : nevets
Minor point on #5 and #6, when accessing an associated array like
$form[class][] = 'someclass';
if the key is a string it should be enclosed in quotes, i.e.
$form['class'][] = 'someclass';
(This is from the PHP documentation.)
------------------------------------------------------------------------
Tue, 23 Aug 2005 14:35:29 +0000 : moshe weitzman
Does this API affect form validation also? Thats the vague impression I
had in my head, but I don't see any validation changed in node or
taxonomy modules. perhaps that part is coming next.
There are reasons to love this patch. But one thing I don't like is the
movement toward arrays and away from functions. Modern editors and IDE's
offer function tips and function completion. These are huge time and
brain savers. They are great for newbies and for experts. It is so
helpful to just type 'form_sel', press tab, and have
form_select('title', 'name', 'value', 'options') printed for you, with
all the arguments. When you define forms in an array instead of
functions, as proposed in this patch, you lose a lot of developer
productivity and friendliness for newbies. Developers are also more
prone to mistakes this way since the editor can't guide them along.
This is the sort of advantage that means nothing to the many people who
just use a plain editor, and means everything to IDE users. Maybe
someone can think of a way to keep the flexibility without losing IDE
productivity.
------------------------------------------------------------------------
Wed, 24 Aug 2005 08:45:53 +0000 : adrian
The api has a drupal_validate_form() function, which does the following
validation :
It steps through each of the elements, and executes any of the valid
properties. An example would be valid => 'username'.
It then calls valid_username($element), which can check for errors.
It then calls $form_id_validate() , which can check for errors between
form elements.
It then (optionally) calls $callback_validate(), which allows you to
have unique form id's , similar to how the example does the node form.
You could create a function $type_node_form_validate(), to validate
only that form, or a theme_$type_node_form() to theme that form
differently.
An example of where this would be used is for CCK, where it will have a
single callback for all nodes created by it.
Errors are flagged using form_error($element). It's different from
form_set_error, in that it also sets the error property of the element,
which I think is more practical than using the globals.
Regarding the IDE discussion, I am on the fence about that, but
definitely leaning towards preferring the arrays over the function
calls. I think that the menu system has proven itself, and that it's
better to be consistent.
The plan for 4.7, is to leave the current form api in , so that all
contrib modules don't need to be ported, but to switch core over to the
new form system anyway, so for the time being .. the old functions will
still exist.
All this would be a non-issue if php supported named parameters, which
is essentially what we are reproducing using arrays.
------------------------------------------------------------------------
Wed, 24 Aug 2005 12:36:08 +0000 : Thomas Ilsche
I agree with moshe, and I think for day to day use the current forms api
does a good job - however on complicated constructions i consider this
to be really useful.
The problem i see is to keep the whole forms api consistent and easy to
learn, any ideas?
I'd be against deprecating the current functions.
About the keyword definitions. I think it should be consistent with for
hook_menu and all its "named parameter" friends, and to at least not
confuse it more define the keywords without the leading underscore.
------------------------------------------------------------------------
Wed, 24 Aug 2005 13:20:13 +0000 : adrian
I'd prefer to make the menu system properties be consistent with the
form system. actually.
I know the conventional logic is that all constants being uppercase,
and the first versions of the form code did stick to that, but the end
result
of the lowercase constants was far more readable code (the underscores
however are a necessity to allow for nesting.)
What i was thinking was, that we could use the conventional form api as
constructors for the form array.
ie:
<?php
$group .= form_textfield(t('File system path'), 'file_directory_path', $directory_path, 60, 255, t('desc here'), null, etc)
?>
turns into
<?php
$form['files']['file_directory_path'] = form_textfield(t('File system path'), $directory_path, 60, 255, t('desc here'), null, etc);
$form['files']['file_directory_path'][valid] = 'directory'; // any other properties that aren't in constructors.
?>
instead of
<?php
$form['files']['file_create_path'] = array( type => 'textfield', title => t('File system path'), default_value => $directory_path, maxlength => 255, valid => 'directory', description => t('desc here') );
?>
and form_textfield turns into
<?php
function form_textfield($title, $value, $size, $maxlength, $description = NULL, $attributes = NULL, $required = FALSE) {
return array( title => $title, size => $size, maxlength => $maxlength, description = $description, attributes => $attributes, required => $required);
}
?>
Benefits :
1) easier to port
2) the ide thingy
Drawbacks :
1) more than one way to do something.
2) all forms will need to be upgraded, since the core form api will
change. ie: breaks all of contrib.
3) Constructors could become unwieldy trying to tend to most of the
parameters that can be set (weight, valid, validation_arguments, etc)
------------------------------------------------------------------------
Wed, 24 Aug 2005 13:47:17 +0000 : Dries
There should only be one way to build forms. Simplicity and uniformity
is king. For now, I leave it up to Adrian to decide what this "one
way" is going to look like. (I like his initial approach. The code is
shorter which saves time too.)
------------------------------------------------------------------------
Wed, 24 Aug 2005 14:24:48 +0000 : chx
I second Dries. The old form API be gone. The IDE is going to be a
problem, yes. A possible approach: the default array have all keys
possible set to NULL or so.
------------------------------------------------------------------------
Wed, 24 Aug 2005 14:28:50 +0000 : Bèr Kessels
I prefer the One Way too. having more ways to do something normally
results in two half-witted ways, instead of one way that works As Best
As Possible.
I like the arrays approach. I love it, in fact. I have a feeling that
the more code in drupal adopts the Array Way [tm], the more power AND
uniformity we get. Just look at the success of array based menus:
powerfull, yet simple to develop with.
But, I have a few hesitations: one is the lowercase CONSTANTS. I know,
this is good for readability, so I lean towards the side of: then just
use lowercase constants. But still, something does not feel right about
it. I think this needs some more though, or comments of others.
Another thing I dislike is the way we use parameters to construct, IMO,
completely different widgets. We should try to not think in terms of
HTML, but in terms of usage and display. In HTML a collapsible form is
similar -or nearly- to a noncollapsible. Same goes for a multi-select
and none-multiselect select.
But where I really think we should have different APIs is for
autofills. They are IMHO not textfields, but a complete separate
widget. Thus they should get a separate API.
And last about the IDEs: allthough I do understand the problem, i
beleive it is a very bad habit to let your code/application/product be
limited, because of the tools you use. If your tools cannot handle
libraries/snippets/etc beyond some function
calls, IMO you should get anoter IDE :). But surely we should not let
us be held back by these limitations in these IDEs.
------------------------------------------------------------------------
Wed, 24 Aug 2005 15:31:14 +0000 : moshe weitzman
Attachment: http://drupal.org/files/issues/form_defaults.patch (2.39 KB)
Um, snippits and macros are not a substitute for function complete.
Snippits and macros are static entities which you manually create in
the IDE and are then available as needed. If syntax changes for an
given snippit, you have to manually change it. Thats just annoying
enough to make you not use these feature at all. By contrast, function
autocomplete just works the second you open a file. If you start
working on a Contrib module you never seen before, the IDE introspects
and is immediately ready for action. So all that nice PHPDoc that
Adrian has written would be nicely used. Not so with snippits or
macros.
If you guys want to see what the fuss about IDE is, download the free
trial of Zend Studio (http://www.zend.com/store/products/zend-studio/)
or Komodo (http://www.activestate.com/Products/Komodo/ - note the small
link for the OSX alpha. I've tried the alpha and it works).
IDEs take a little while to get configured and get comfortable ...
Create a project and start typing some common functions. See how the
params show up and all our PHPDoc is there for your use. You can also
quickly navigate your project via function names, and skip to the spot
where a given function is defined. It is a tremendous time saver, and
bug preventer. The IDEs above also offer a debugger which lets you step
through your code and set breakpoints. Want to know the current value of
a variable - just hover over it in your debugger. Once you've gotten the
hang of this, you will never debug using print statements again.
I see nothing wrong with designing Drupal so that it uses those PHP
language features that are friendly to IDEs. Namely, functions,
classes, constants, etc. Arrays are very flexible, but that flexibility
comes at a price.
------------------------------------------------------------------------
Wed, 24 Aug 2005 16:34:41 +0000 : chx
Attachment: http://drupal.org/files/issues/forms_chx.patch (15.59 KB)
Like we have core.php for hooks we can have form.php for form API and
let that help your hand with any IDE.
Here is an updated version of Adrian's patch. User login block
reworked. I move on to other parts of user.module. I also created a
theme_password() function, so I will post form.inc as well.
------------------------------------------------------------------------
Wed, 24 Aug 2005 16:49:05 +0000 : chx
Attachment: http://drupal.org/files/issues/form_0.inc (21.01 KB)
------------------------------------------------------------------------
Wed, 24 Aug 2005 22:23:06 +0000 : gordon
+1 I like this patch a lot. but 2 things.
* The testarea hook is not supported. This needs to be implemented.
* the existing form_*() need to be changed to use the theme_*() so that
there is only one place that form items can be created.
I think this will be great for theme developers and it will be easier
to build forms for module developers.
Great job.
------------------------------------------------------------------------
Fri, 26 Aug 2005 09:27:08 +0000 : adrian
Attachment: http://drupal.org/files/issues/form_1.inc (24.73 KB)
Here is an updated version of the patch.
1) admin / settings has been rewritten
2) Added form_radios and form_checkboxes and form_select and a few
others
3) Very strict validation now .. the drupal_get_form function
automatically validates any input .. no way to skip that. If it doesn't
produce errors, it calls $form_id_execute(), or the optional
$callback_execute().
4) Started the first part of the implementation of the multiple
keyword, which is when an element can have multiple values. An example
of this would be the 'files' element from upload.module , where more
files get added, and also the primary / secondary links configuration
of phptemplate.
Still needs to be done : The clean_url validation, and any other module
with a _settings hook. I haven't integrated chx' user module work at
this point.
------------------------------------------------------------------------
Fri, 26 Aug 2005 09:48:14 +0000 : adrian
Attachment: http://drupal.org/files/issues/forms_0.patch (37.68 KB)
Here's the patch.
I'm also still busy with the filter format, which has unique
requirements and needs to be a filter_format element type (gets rid of
the in-line html)
------------------------------------------------------------------------
Sun, 28 Aug 2005 02:52:59 +0000 : yogadex
One thing I like about the current API is that you can inject arbitrary
HTML in the middle of a form if you see fit. It's not obvious to me
how to do that with the new API. Is there a way?
Could I, for example, arrange my form fields in a table with two or
three columns?
------------------------------------------------------------------------
Sun, 28 Aug 2005 07:26:13 +0000 : naudefj
I would also like to be able to display forms in HTML tables. Here's a
great article explaining how forms can be styled using tables -
http://www.cs.tut.fi/~jkorpela/forms/tables.html
------------------------------------------------------------------------
Sun, 28 Aug 2005 08:25:06 +0000 : adrian
EVERY form will now have a theme function, as every form now has it's
own (unique) form-id.
You can create a theme_my_form_id() function if you are a developer,
and require the form to be differently layed out (like for instance,
adding all the element outputs to a table).
You can create a mytheme_my_form_id() if you are a theme developer,
that can override the form layout. (as is shown by the current
node_form)
[?
function theme_node_form($form) {
$output .= '';
$output .= '';
$output .= '';
$output .= form_render($form['author']);
$output .= '';
$output .= '';
$output .= form_render($form['options']);
$output .= '';
$output .= '';
$output .= '';
$output .= form_render($form_render);
$output .= '';
$output .= '';
return $output;
}
?]
You can create a phptemplate stub to load it from a template.
<?php
function phptemplate_node_form($element) {
return _phptemplate_default('node_form', $element);
}
?>
and the current node_form template being :
[?
<?php
print form_render($form['author'])
?>
<?php
print form_render($form['options'])
?>
<?php
print form_render($form)
?>
?]
NOTE: As a developer, you can even remix forms you didn't design.
Infact, it gives you complete themeability over everything.
------------------------------------------------------------------------
Sun, 28 Aug 2005 08:44:27 +0000 : adrian
Attachment: http://drupal.org/files/issues/forms_presentation.pdf (78.63 KB)
I hate the php filter ..
here's the theme_ function ..
i don't have time to escape the template right
<?php
function theme_node_form($form) {
$output .= '<div class="node-form">';
$output .= '<div class="admin">';
$output .= '<div class="authored">';
$output .= form_render($form['author']);
$output .= '</div>';
$output .= '<div class="options">';
$output .= form_render($form['options']);
$output .= '</div>';
$output .= '</div>';
$output .= '<div class="standard">';
$output .= form_render($form_render);
$output .= '</div>';
$output .= '</div>';
return $output;
}
?>
I am attaching my presentation from drupalcon in portland.It has all
the examples, although stuff like the validation has changed slightly.
------------------------------------------------------------------------
Sun, 28 Aug 2005 16:53:48 +0000 : adrian
Call for help :
Code freeze for Drupal 4.7 is coming very quickly , and a lot of work
is still needed to get this functionality in before the code freeze
happens.
As this patch is incredibly important (even if only considered from a
security point of view), we need people to help us port all the forms
in
Drupal at the moment.
If anyone is interested in helping, could you please contact me, so we
can coordinate.
------------------------------------------------------------------------
Sun, 28 Aug 2005 22:19:40 +0000 : killes(a)www.drop.org
You can assign me a module to conver as soon as my revisions patch is in
core. I think I'd like to convert profile.module.
------------------------------------------------------------------------
Sun, 28 Aug 2005 23:41:06 +0000 : yogadex
Regarding #24:
Now I see why you keep the [printed] flag during node_render.
Should this line:
$output .= form_render($form_render);
read:
$output .= form_render($form);
??? (That is, I don't see where variable $form_render is coming from).
Regarding #25:
Must every form be converted in order to include this? Drupal 4.7 will
still support the old forms API, yes?
I see that this was discussed early in the thread. For those of us
with custom modules life would be a lot easier if the old form api
sticks around until say a 5.0 release.
------------------------------------------------------------------------
Mon, 29 Aug 2005 00:02:09 +0000 : killes(a)www.drop.org
@yogadex: To those of us who roll out the actual security releases till
4am in the morning the disappearance of the old api with 4.7 will
provide a lot more sleep. We win, backwards compatibility sucks.
------------------------------------------------------------------------
Mon, 29 Aug 2005 03:50:27 +0000 : adrian
The old form api will remain in core for a maximum of one release.
another 6-9 months to convert your modules is more than enough time...
plus most of the new uber features (cck etc) are going to require you
to upgrade to the new system.
Meanwhile, new functionality like the install wizards are going to
require you to use this form api.
------------------------------------------------------------------------
Mon, 29 Aug 2005 03:59:41 +0000 : adrian
And regarding the form_render question .. it's not a variable, it's a
small recursive function.
the theme function is passed the entire form tree. The designer can
choose to remix the form however he chooses. Since elements are nested,
and for instance the fieldsets, are named .. you can use
form_render($form['elementname']) , which will print that element, and
set the printed flag so it won't be printed again.
Like in the example where the author and options fieldsets are
seperated out from the form and displayed seperately.
form_render($form) in turn will print anything that hasn't been printed
yet.
if you wanted to seperate out the author field on the page, you could
use form_render($form['author']['name']) , and it would print that
element wherever, and when you then called form_render($form['author'])
or form_render($form); , it would not be printed again.
------------------------------------------------------------------------
Mon, 29 Aug 2005 04:24:47 +0000 : yogadex
@killes: I certainly don't want you losing sleep ;) I'm glad to hear
the new and old will coexist for at least one release. (Unless there's
a security issue - that's another story)
@adrian: I understand the form_render() function and it's nifty. But
there is a typo in both your attached presentation and #24 above, where
"$form_render" is written and it should be "$form", if I am reading
correctly. Thanks for explaining.
------------------------------------------------------------------------
Tue, 30 Aug 2005 21:14:37 +0000 : chx
Attachment: http://drupal.org/files/issues/forms_1.patch (46.21 KB)
I merged and rerolled against current HEAD.
------------------------------------------------------------------------
Wed, 31 Aug 2005 13:59:30 +0000 : m3avrck
Minor include bug in the node.module patch:
define('NODE_NEW_LIMIT', time() - 30 * 24 * 60 * 60);
+include_once 'includes/form.inc';
that include_once is missing the './' part.
------------------------------------------------------------------------
Thu, 01 Sep 2005 00:25:22 +0000 : ax
that second
<?php
+include_once 'includes/form.inc';
?>
shouldn't be changed but *removed*, because it is a duplicate of 8
lines above.
------------------------------------------------------------------------
Thu, 01 Sep 2005 00:31:52 +0000 : m3avrck
ah yes, an even better catch than mine, touche!
------------------------------------------------------------------------
Thu, 01 Sep 2005 01:14:12 +0000 : chx
Not so. The first should be removed 'cos it is hard to patch against the
cvs id :)
------------------------------------------------------------------------
Thu, 01 Sep 2005 01:42:27 +0000 : ax
allright, ok, of course, the first one.
what would be even better would be a working patch. the ones attached
(last one and before) don't include form.inc, generate some duplicate
functions that result in "PHP Fatal errors: Cannot redeclare
functions()" (system_elements(), theme_node_form()), and "PHP Warning:
Call-time pass-by-reference has been deprecated - argument passed by
value"s. chx: are you working on this?
i would much like to help testing the new forms api and getting it into
4.7 because thats exactly what i need for a bigger project at work where
we are considering using Drupal as base framework. the one showstopper
is the extensability of Drupal forms ...
thanks for all the work done up to now, anyway.
------------------------------------------------------------------------
Thu, 01 Sep 2005 02:38:08 +0000 : jvandyk
Attachment: http://drupal.org/files/issues/forms_2.patch (47.72 KB)
Here's an updated patch against HEAD that is mostly working. Karoly
provided the updated node.module. For me, there seems to be an issue in
form.inc in the recursive _form_builder() function where it is recursing
even when $element is not an array but is a scalar.
------------------------------------------------------------------------
Thu, 01 Sep 2005 02:43:01 +0000 : m3avrck
Not to nitpick, but in node.module, the patch has +include_once
'includes/form.inc'; but should read +include_once
'./includes/form.inc'; for consistency and performance :)
------------------------------------------------------------------------
Thu, 01 Sep 2005 03:24:16 +0000 : webchick
I don't know if this is just the Drupal newbie in me talking, but I am
getting quite a few errors with this patch (Drupal HEAD, up-to-date as
of about 5 minutes ago). I've uploaded the form.inc from #19 and
applied the forms_2.patch from #38, and I'm getting errors like the
following:
*admin/settings:*
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 511.
*admin/comment/configure, admin/settings/user*:
warning: Missing argument 2 for system_settings_form() in
$DRUPAL_ROOT/modules/system.module on line 733.
*node/add/[anything]*:
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 406.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 403.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 411.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 406.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 403.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 411.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 406.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 403.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 411.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 406.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 403.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 411.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 406.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 403.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 411.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 435.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 448.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 435.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 448.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 435.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 448.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 435.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 448.
warning: Invalid argument supplied for foreach() in
$DRUPAL_ROOT/includes/form.inc on line 435.
warning: Cannot use a scalar value as an array in
$DRUPAL_ROOT/includes/form.inc on line 448.
---
Any ideas?
------------------------------------------------------------------------
Thu, 01 Sep 2005 03:35:44 +0000 : jvandyk
That's why I said "mostly working." :)
m3avrck, sorry, I diffed against the copy where I hadn't corrected the
./ thing.
------------------------------------------------------------------------
Thu, 01 Sep 2005 09:47:05 +0000 : adrian
Good morning guys.
I am looking at those errors now, and i will upload an updated
forms.inc in a short while.
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:46:02 +0000 : adrian
Attachment: http://drupal.org/files/issues/form_2.inc (26.76 KB)
Here is an updated patch and includes/form.inc.
Changes :
1 ) Introduce a global $form_variables array, which works exactly like
$_POST['edit'] used to, except the values in it are filtered through
the form system and only values that have form elements will be in it.
Each form being processed on a page resets this array, so they won't
interfere with each other.
2) introduce a form_id hidden that gets submitted with the page. This
means it can intelligently decide which form to process.
3) Fix the bug above. I wrapped the offending lines in if is_array() ,
HOWEVER .. at the point I call that code I am fairly certain I am only
working on arrays, hence I need to look it over more
4) fixed the user login form on /user. The user login block still
doesn't work
5) Added a default css class (and some temporary css in drupal.css) for
everything rendered with the form api. All other forms will have a thick
red border if they have not been ported.
6) fixed a bug with element_info where it was setting the wrong
defaults and then messing up the first form element because of it.
7) finished the filter module 'filter_format' element. Filter forms are
printing now.
8) Added the 'weight' element. Could be cool to theme this with
something ajaxy, right ?
9) Fixed some bugs with admin/system
10) Updated the following modules : Book, Page, Story, Blog, Node,
Taxonomy and a fixed user form.
11) Moved the inclusion of form.inc to it's rightful place in
drupal_bootstrap_full.
12) Added a bunch of comments to places. We will still have to go
through all the comments and make sure they all 'parse' =)
The node form is not actually working or validated at present, and if
someone could take a look at that , I'd be much obliged. It's important
to get used to the new (strict) workflow for forms. It generally means
having to shift some code around, and it makes it harder to write 'bad
code'.
------------------------------------------------------------------------
Thu, 01 Sep 2005 23:05:56 +0000 : adrian
Attachment: http://drupal.org/files/issues/forms_3.patch (54.13 KB)
here's the patch
------------------------------------------------------------------------
Fri, 02 Sep 2005 05:48:44 +0000 : Dries
/introduce a form_id hidden that gets submitted with the page. This
means it can intelligently decide which form to process./
Does this mean people could change the ID before they submit the form?
I'm not sure that is a good idea.
------------------------------------------------------------------------
Fri, 02 Sep 2005 10:04:21 +0000 : adrian
The access check is tied to the page, not to the form.
If the page has 2 forms on the page, this check is to see which of the
two he submitted.
Changing the form_id in the hidden will do nothing but cause the form
not to execute, at all.
------------------------------------------------------------------------
Fri, 02 Sep 2005 14:04:45 +0000 : m3avrck
Running this patch I'm getting this error Fatal error:
_drupal_bootstrap_full() [function.require]: Failed opening required
'./includes/form.inc'
Didn't see the file anywhere in the patch, unless I missed something...
------------------------------------------------------------------------
Fri, 02 Sep 2005 14:44:22 +0000 : adrian
drupal.org renames the files to avoid conflicts.
it's attached as form_2.inc
------------------------------------------------------------------------
Sat, 03 Sep 2005 03:06:21 +0000 : asimmonds
Should there be "return_value" and "delta" property constants defined in
form.inc?
------------------------------------------------------------------------
Sat, 03 Sep 2005 06:32:57 +0000 : adrian
Attachment: http://drupal.org/files/issues/form_3.inc (27.33 KB)
Well spotted.
Added those defines.
------------------------------------------------------------------------
Sat, 03 Sep 2005 11:53:53 +0000 : nicopepe
Attachment: http://drupal.org/files/issues/modif_Form_Inc.php (2.52 KB)
Hello
I am a newsbe and i am vey concern about the form project. I want first
to congratulaed for all this nice work. I propose this code in the
attached file. Tell me what you think, and i'll finished it.
------------------------------------------------------------------------
Sun, 04 Sep 2005 14:57:30 +0000 : nicopepe
Hello again,
I see no reaction to my proposition. Maybe not interresting code ? My
aim with that code is :
- Open all the possibilities of attribute in an input balise.
- Not multiply the number of constant (The key of an element is an
attribute name, define by the programmer)
- Check within drupal_attributes function if these attributes are
correct in an INPUT Balise.
- Include a javascript code insertion for form client validation.
In fact i would like to create later on, a module with generic
javascript for client validation (Date checking, required fields, ...).
I have this in mind when i proposed this code.
Any answer this time ?
------------------------------------------------------------------------
Sun, 04 Sep 2005 15:11:41 +0000 : clydefrog
nicopepe, it is difficult to tell what your changes are because you have
not submitted a patch. Take a look at http://drupal.org/patch. Your code
might get a better response that way.
------------------------------------------------------------------------
Sun, 04 Sep 2005 18:25:23 +0000 : Dries
As an aside, it might be worth looking into the XForms [1]. They have a
short introduction for HTML authors [2] in case you don't feel like
reading the entire specification. A friend of mine is a member of the
XForms working group and told me someone was working on a JavaScript
implementation of XForms.
More importantly, XForms have (client-side) validation build-in. For
example, to specify a valid range you use:
<range ref="volume" start="1" end="10" step="0.5"/>
Similarly, XForms have the ability to state that a value must be
supplied before the form is submitted, have a notion of typed values
(eg. you can specify that you expect an URL, integer or date to be
entered). XForms also include stuff to introduce wizard-like behavior
of filling in a form in stages, include functionality to dynamically
add fields (eg. like when you run out of textfields for poll
questions), etc. A lot of the stuff we have been talking about. :-)
It might be useful to check if our developer API would map gracefully
on XForms. It would impose that we are XForms-ready (future proof).
It's especially interesting because Mozilla is working on a XForms
implementation [3]. (XForms 1.0 support will be included in Firefox
1.1. You can test it using the nightly Firefox builds.)
[1] http://www.w3.org/MarkUp/Forms/
[2] http://www.w3.org/MarkUp/Forms/2003/xforms-for-html-authors
[3] http://www.mozilla.org/projects/xforms/
------------------------------------------------------------------------
Mon, 05 Sep 2005 11:15:16 +0000 : nicopepe
As a newsbe, i have not install any patch module Yet. So i will do it.
For Dries, i will have a look on this Xforms projet, but i just wonder
if it is MS compatible (80% of market) ?
Maybe before adding this code I should have write my ideas. I think
this new form API should consider having an easy way to perform
database update as well as new records adding. So in my view, 2
specials “events” are needed :
- call an user defined procedure BEFORE the Form is Loaded (non
existing yet ?) to read database and set value to the form.
- call an user defined procedure AFTER the Form is validate
(drupal_execute_form)to write to the database update or new record.
I agree with Adrian to use arrays. They allowed flexibility. I think
using $form and $element arrays with others keys than theses defined
with property constants is a good way. Property constants are used for
regular Drupal core functionality, and user defined keys values used
for extra functionalities. This means that the $form array should have
a well documented structure to be able to extend it.
I see here three examples:
- Use all JS events (as my code suggest) for client validation.
examples :
$form['onSubmit'] = "js function name or code...";
$element['onkeydown'] = "js function name or code...";
- Create an $element[‘NonValidationMessage’] Key used in
drupal_validate_form user function to return to the user when the
validation of an element is not OK (This message could also be used in
JS in client validation).
- Possibility for a programmer to input in the $element array the name
of the database field and database table to store the input value in
case of correct validation. Exemple :
$form['tablename'] = "mytable";
$form['idValue'] = "value";
$form['idName'] = "myid";
$element['Databasefieldname'] = "myfieldname";
Then create an function to create automatic SELECT STATMENT to read
the value before display the form ( When we need to modify value stored
in a database) or INPUT SQL statement after validation,
I hope i am clear with my ideas. Maybe some of them could be used
already in the core.
------------------------------------------------------------------------
Mon, 05 Sep 2005 11:47:08 +0000 : Dries
nicopepe: I'm not saying we should implement XForms at this point. I
meant to say that (i) XForms looks like an emerging standard and that
(ii) the XForms API tries to solve the same problems we are trying to
solve with the new form API. It is worth investigating XForms. Being
able to generate XForms-compliant code with minimal changes is a nice
extra, but certainly not a must at this point. Hope that clarifies my
comment.
------------------------------------------------------------------------
Mon, 05 Sep 2005 12:01:28 +0000 : Junyor
Dries: It'd probably be more interesting to look at Web Forms 2:
http://www.whatwg.org/specs/web-forms/current-work/. It's geared
toward the Web, whereas XForms isn't. Web Forms 2 is also more likely
to see native implementations.
------------------------------------------------------------------------
Mon, 05 Sep 2005 12:33:38 +0000 : killes(a)www.drop.org
Dries: In my book we are trying to solve a securoty issue woth this
patch: the case of inserting bogus fields into our html forms. Client
side validation is noce from a usability pov but we are trying to do
server side.
------------------------------------------------------------------------
Mon, 05 Sep 2005 14:02:58 +0000 : Dries
Gerhard: it's obvious that client-side validation is no replacement for
server-side validation. :-)
Anyway, instead of looking years ahead, let's focus on the patch at
hand.
------------------------------------------------------------------------
Mon, 05 Sep 2005 15:06:27 +0000 : nicopepe
Client side validation will never replace server side validation for
security reason !
I do understand that client side is not yet a priority, but please let
doors open for module developper if they want to implement Client side
validation in their module.
------------------------------------------------------------------------
Mon, 12 Sep 2005 17:55:57 +0000 : adrian
Attachment: http://drupal.org/files/issues/forms.tar.bz2 (0 bytes)
Here is an updated, and more complete version of this patch.
I had a bit of a SNAFU with the multiples code, and needed to refactor
it to work properly.
Changes in this version :
1) Introduced a 'process' property. This is used by the radios and
checkboxes elements, to call 'expand_radios' and 'expand_checkboxes'
functions which turn the single element into a set of elements.
2) admin/themes screen done.
3) deprecated fix_checkboxes, since it was no longer needed.
Checkboxes and radios work perfectly now ( they were broken
previously).
4) The way to access the form value is through a global $form_values
array, that is set up to the structure of the form. It's no longer
flattened, and it works perfectly.
5) Introduced an array_walk_recursive function (copied from PHPCompat
in PEAR) that allows you to step through the values and do something
with each key. (admin/settings uses this property).
6) Moved the old form api into a legacy.inc
7) Did a bunch more forms.
In this tarball you will find the 2 new files (legacy.inc and form.inc)
and the current patch.
------------------------------------------------------------------------
Mon, 12 Sep 2005 17:57:30 +0000 : adrian
Attachment: http://drupal.org/files/issues/Forms.rtf (14.02 KB)
Here's the status document of all the forms (rtf export from
omnioutliner)
It's incomplete, as I haven't finished cataloging all the forms yet.
------------------------------------------------------------------------
Mon, 12 Sep 2005 18:01:25 +0000 : m3avrck
No patch in that zip :)
------------------------------------------------------------------------
Mon, 12 Sep 2005 18:54:08 +0000 : m3avrck
Also, adrian, is there a way to add events or set variables in
javascript, if the submit button on a form has been clicked? Need this
functionality for a patch about to (hopefully!) make its way into HEAD
soon.
------------------------------------------------------------------------
Mon, 12 Sep 2005 20:51:26 +0000 : adrian
Attachment: http://drupal.org/files/issues/forms.patch.txt (108.18 KB)
I'm sure it's possible, but I think we prefer attaching events and the
like onto forms using the id attribute.
here's the patch file.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:07:59 +0000 : adrian
Attachment: http://drupal.org/files/issues/Forms_0.rtf (22.47 KB)
Here's a complete list of the forms in drupal that still need changing.
Everything that has already been done is not on this list (ie: all the
node modules, and the like)
Some of the forms mix normal output, and form output in the same page,
and switch out depending on certain requirments (the
aggregator_list_page was an example of this), making it a fairly
intensive process to seperate the two pieces of functionality into
clean code.
If something is rated simple, it means it only involves changing the
functions to arrays, and either returning the array, or returning the
final form using drupal_get_form('form_id', $form);
If we want any chance of getting this into core before 4.7, I am going
to need some volunteers to handle tackling these forms for me. Once
again, I am looking for a few good men (where men == female OR men ==
male .. obviously. =) ).
I am going to be concentrating on the really complex forms, and the
theme('confirm') form, which is required by a fair amount of things.
------------------------------------------------------------------------
Tue, 13 Sep 2005 03:38:20 +0000 : Jeremy(a)kerneltrap.org
I thought I'd give this patch a quick try, but ran into a couple
problems. First is very minor, it makes some changes to settings.php
that it shouldn't -- something to clean up before the final version.
Second, it adds includes for form.inc and legacy.inc, but the files
themselves are not actually included in the patch.
------------------------------------------------------------------------
Tue, 13 Sep 2005 11:53:05 +0000 : adrian
They are in the tarball i uploaded earlier.
------------------------------------------------------------------------
Tue, 13 Sep 2005 12:07:31 +0000 : Jeremy(a)kerneltrap.org
I tried that last night too, but the earlier tarball was 0 bytes and
contained no files:
Attachment: forms.tar.bz2 (0 bytes)
------------------------------------------------------------------------
Tue, 13 Sep 2005 12:22:52 +0000 : adrian
Attachment: http://drupal.org/files/issues/forms.tar_0.bz2 (27.82 KB)
That is the weirdest thing ..
locally :
Benton:~/dev/forms adrian$ ls -l forms.tar.bz2
-rw-r--r-- 1 adrian adrian 28489 Sep 12 15:30 forms.tar.bz2
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:03:17 +0000 : adrian
Attachment: http://drupal.org/files/issues/Forms.html (10.02 KB)
Html version of the status list.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:12:22 +0000 : Dries
Guys, please take a look at the status page. If we want the new form
API in Drupal 4.7, we'll have to work together pretty much non-stop to
complete it! Care to help? Pick an item, migrate to the new form API
and upload a patch.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:26:09 +0000 : Bèr Kessels
Can we not exclude this from the code freeze? I mean that we now (last
days) focus on all these outstanding patches for 4.7. and then have
acollaborative effort on this forms patch. That will at least give us
the benefit that we can aim on a stable target. Instead of junting on a
still changing core. From what I see a lot of people are focussing very
hard at their babies in the queue, to try to push them into 4.7 (that
counts for me anyway) So they will not spend any time on the form api
yet.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:41:45 +0000 : webchick
For what it's worth, I agree with Bèr. I've been picking away at
comment.module to make it compatible with the forms API, but it's very
difficult since the patch needed to enact all these changes differs
from day to day (today I applied the .tar'ed version and still am
getting errors with a few hunks which I need to troubleshoot first in
order to make sure my changes aren't causing errors).
Once things are more "settled" with the rest of core, it seems like it
would be much easier to make a more "stable" patch as a starting point,
which would thus ease efforts on the part of the rest of us to get the
changes made. This patch represents a HUGE change, and I worry about
the idea of trying cramming it in at the last minute.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:54:29 +0000 : Souvent22
I totally agree with the above comments. With so many chagnes happening
so fast, we wake up to a new core each morning, and must make changes
accordingly. I don't know what the core will get 'stable' enough before
the freeze to really hack through what needs to be done in an efficient
manner. We all seem to agree this *needs* to happen, but perhaps a
better plan of *'how'* it should happen since it is such a big change
would make things easier, more stable, and not make it seem like we're
trying to cram a square block through a round hole.
1
0
Returned Mail: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by drupal-devel@drupal.org 15 Sep '05
by drupal-devel@drupal.org 15 Sep '05
15 Sep '05
The following message from Bèr Kessels <drupal-devel(a)drupal.org> was not authorized for entry in the 01Lists.com forum. Posts can only be made by registered users of the forum.
From: Bèr Kessels <drupal-devel(a)drupal.org>
To: drupal-devel(a)drupal.org
Subject: [drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
****************************************
Issue status update for
http://drupal.org/node/30220
Post a follow up:
http://drupal.org/project/comments/add/30220
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: m3avrck
Updated by: Bèr Kessels
Status: patch (ready to be committed)
In general I dislike the feature. Konqueror somehow dies this, and that
is the way it /should/ be. Browsers should take care of this, not the
web app.
But, since it is only konq. this patch has a good enough additional
value :)
I would like to see this patch tested with, tinyMCE and HTMLAREA, at
least. For I am quite sure this will break these modules so bad that
they are near unusable.
Which brings me to the next point: using the textarea hook a simple
module could take care of this.
I would very very much prefer this living in a contrib, or even a core
module. Just not "enforced" on me.
And the last point: if this is for core, please use that textarea hook
too. This is where that hook is for: extending the textareas.
Bèr Kessels
Previous comments:
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck
How many times have you been posting a comment or working on a new post
when you click on a link in another application and you navigate away
from the content you are editing, losing all of your changes?
Well I say we introduce a script, much similiar to that of Blogger,
that uses the Javascript window.onbeforeunload handler to prevent this
from happening. We can set this handler to call a function that
compares the contents of all forms when the page loads and then quickly
compares that to the current contents just before the user is about to
leave the page. If they have changed, we should prompt the user so they
don't lose their work. This would save many headaches and degrade nicely
for users without Javascript.
Additionally, this script should work with the newly introduced
JS-based upload feature to prevent navigating away as a file is being
uploaded as well.
If I have sometime I'll start work on a patch tomorrow, just wanted to
get the idea into the queue right now :)
------------------------------------------------------------------------
Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH
Sounds like a great feature to me. We regularly get authors and editors
telling us that they've lost the last hour of work by accidentally
navigating away, and I've even just had one of them email me about it a
few minutes ago, so it'd be great if you could put something together
for this.
Cheers
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC
Sounds great to me, too. I often have several tabs open doing stuff and
it's easy to lose my place and leave a page with unfinished edits.
Would this alert you when you're going to close an unsaved tab/window
as well?
Michelle
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles
Great for us multitaskers that rush and click the wrong thing
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:36:52 +0000 : chx
Very strong -1 as this is IE only.
------------------------------------------------------------------------
Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium
Konqueror already does this, +1 for the idea in general.
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:07:21 +0000 : webchick
Yeah, I think chx is correc that onbeforeunload is an extension
developed by MS [1] (it's not in the ECMAScript specification [2].
However, I found out that this is implemented in Mozilla since 1.7 [3]
(which corresponds to Firefox 1.0, I think?) and I've tested
Blogger.com's version of this functionality in:
Firefox 1.0.6 (working)
Firefox Deer Park Alpha 1 (working)
Opera 8.0.2 (working)
Safari 1.3 (does NOT work)
Safari 2.0.1 (working)
So it seems to be a 'de facto' standard if nothing else.
[1]
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun…
[2]
http://www.ecma-international.org/publications/standards/Ecma-262.htm
[3] http://www.mozilla.org/status/2004-03-01.html
------------------------------------------------------------------------
Mon, 12 Sep 2005 15:09:15 +0000 : webchick
Also, I should clarify... that "does NOT work" in Safari doesn't display
any errors or anything, it just doesn't save the form results as
expected (so pre-Tiger Safari users are going to be used to this). So I
don't see any harm in including this functionality since it seems to
degrade gracefully in non-supporting browsers.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes)
And a patch is ready! Code was based on many example, including this [4]
and this [5] along with adding my own thoughts and what not ;)
[4]
http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O…
[5] http://www.blogger.com/app/scripts/formcheck.js
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB)
And here is the JS to throw in misc/.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB)
Better JS file attached without tabs. Also, tested and working great in
FF and IE6. Doesn't work in Opera 8, however no errors are present, I
believe this is just because the event handler is not supported. If
there is a work around for Opera, please let me know!
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB)
One more try with those tabs.
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman
hmmm. i usually find these prompts annoying. any chance we can attach
the behavior only to the node and comment forms? those are the "high
risk" areas. what do others think?
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck
Yes, the JS is only loaded on the node edit/create pages, won't affect
any admin/login/etc form. Also, the prompt is unobstrusive and if you
goto save a node you don't get a warning that the contents have changed
because you are explicity saving the node (hence no reason!).
------------------------------------------------------------------------
Mon, 12 Sep 2005 21:53:01 +0000 : Dries
I want some JS-folks to review the code.
------------------------------------------------------------------------
Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB)
After talking with Drumm on IRC, this patch makes the integration with
Drupal much simpler and it's off by default. Only turned on for the
node edit/change page right now but any module/form can easily add this
by passing a 'TRUE' parameter.
------------------------------------------------------------------------
Mon, 12 Sep 2005 23:38:35 +0000 : nedjo
+1 on idea, js looks generally good, a few suggestions:
1. Instead of writing onsubmit via PHP, use a js addLoadEvent call.
2. Shouldn't the return false in isElementChanged be at the end
(outside the switch block)?
3. It's be nice to find a way to make messages like 'You have unsaved
changes.' translatable. Pass in global js variables via a t('') call?
4. onbeforeunload event should probably be in a if isJsEnabled test,
and should parallel drupal.js's event adding (see addLoadEvent).
5. Comments should be in standard format and in present tense, e.g.,
/**
* Checks to see if a form has been changed after the page loads
*/
6. e_ should be just e to match other js files, e.g., autocomplete.
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB)
Ok here is an updated JS file.
A few notes, I couldn't get the addSubmitEvent() in drupa.js to
reliably work so I had to set the isSubmit() true in the PHP creation
of the form. If you look at Blogger.com, they do this as well... so
either we both missed an obvious way to do this, or that is the most
practical. Hopefully some wise JS gurus will chime in with an answer.
Same goes for onbeforeunload event, which is completely different then
addEvent defined in drupal.js.
As for the t('') I agree this would be useful but I'm not sure of the
best way to do this. One way would be to put in form() a t('') passed
in, and then write this to a var in formcheck.php which returns a JS
file type. Thoughts?
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:06:44 +0000 : Junyor
FWIW, Opera doesn't support the onbeforeunload event. However, this
issue doesn't affect Opera anyway: form contents are retained in
history as long as the page isn't closed. IOW, this feature doesn't
work in Opera, but it isn't needed either.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB)
Updated JS styling and JS-killswitch after talking with Unconed on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck
Just to build on Junyor's comment [6] this functionality is built into
Opera 8 and it *doesn't* produce any errors.
[6] http://drupal.org/#comment-44066
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB)
Updated patch to fix possible problem of overwriting onsubmit event
handler thanks to Thox on IRC.
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:13:59 +0000 : webchick
I tested this and can confirm that it works in *both* versions of Safari
that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So
looks like this code goes one better over the Blogger.com stuff,
because their stuff doesn't workk in 1.3. Nice job! ;)
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:25:21 +0000 : m3avrck
Well I'm gonna set this to commit then. Tested and working on IE, FF,
Safari. Doesn't work or break Opera or Konqueror but not needed in
these cases. Thox and Unconded have offered thoughts and I've modified
code as needed. Doesn't seem like there is anything else left except a
commit :)
1
0