Skip to content

Conversation

@jdespatis
Copy link
Contributor

In fact, I've run into problem with ezscriptmonitor:

  • the name to store the script is only 50, well why not pushing to 255 to be sure it's enough ?
  • the command is only 255, why not putting text instead to increase the max length ? (in fact 255 is really not enough for my need, as I send a serialized array as a param to the sheduled script, and it can be very verbose...)

Some remarks:

  • the files sql/oracle/schema.sql and sql/postgresql/schema.sql have not been updated, as I cannot generate them with the .dba with my not so recent eZPublish install
  • I've put text as type in the .dba, dunno if it's a type none on all dbms

Feel free to comment ;)

@glye
Copy link
Member

glye commented Dec 1, 2011

Name: Enough for what? The name is just there as a human readable identifier for the script in the monitor table. Parameters etc should not go here. What do you need 255 chars for?

Command: If we change it we must change it for Oracle and Postgres too, so the patch is incomplete. I see your point, but if we go that big we risk hitting the max length of unix command lines. I found that the limit is at least 2048 on POSIX systems, it may be larger on some systems but there is no guarantee about that. We would need a mechanism to protect against overflow.

@jdespatis
Copy link
Contributor Author

Well, as the aim was to increase the size of the command, why not do the same with the name as, in term of sizes a varchar(50) is the same as varchar(255) for a string smaller than 50 (in mysql)

But, I agree 50 should be enough for the name

However for the command, 255 is really too short...

Maybe it's possible to increase to 1024 instead of 255? it should be enough

Of course, the solution would be to have no limitation at all (to avoid to know the minimum size of all OS supported by eZPublish)
Maybe a temporary file, but the scheduled script should be aware of it in order to clean it, maybe $_ENV, but not sure it's ok on windows

Any idea for a perfect solution with no limitation?

@jdespatis
Copy link
Contributor Author

Limit seems to be (at least) 2047 on Windows
http://blogs.msdn.com/b/oldnewthing/archive/2003/12/10/56028.aspx

So no risk to increase the command(255) to command(2000) ? What do you think about it @glye ?

@glye
Copy link
Member

glye commented Dec 1, 2011

Yes, I think 2000 should be pretty safe.

As for a perfect solution with no limitation, I don't know. I think that will always depend on the command, don't see how that can be done by the extension.

@jdespatis
Copy link
Contributor Author

Ok, and for the name, you want to keep with the 50, or maybe increase a little ?

In fact, for now, I'm filling this name with: "myExtension some summary"
That way, in the listing of scripts that are monitored, I can see quickly which extension has launched the script, and a description of it

As a result, 50 is ok, but may be too short if using a big name for an extension with a clear summary (that may be long)

You might object that 50 may force the developer to be concise in his description, which may be a good thing also :)

But why not let the developer decide to be verbose or not for this name?

@glye
Copy link
Member

glye commented Dec 1, 2011

I don't have strong opinions about the name length. My concern is that the monitor table should be easily readable: http://projects.ez.no/var/plain_site/storage/images/ezscriptmonitor/70039-1-eng-GB/ezscriptmonitor.png
...but then again, having a description here could be good too. I guess I'm ok with 255.

@jdespatis
Copy link
Contributor Author

Ok, so new commit with command pushed to varchar(2000). Can you review this patch @glye please?

The patch is complete now, as modifications have been done to all dbms

BUT, it has been coded blindly for oracle, postgresql, I've no skill on the available types on those dbms...

@glye
Copy link
Member

glye commented Dec 2, 2011

I have checked the varchar limits for Oracle (4000 bytes) and Postgres (1 GB), so they seem ok. I'm fine with the patch as it is now. As far as I'm concerned it can be merged.

@andrerom
Copy link
Contributor

andrerom commented Dec 2, 2011

Will the increased name size potentially break gui's?

@glye
Copy link
Member

glye commented Dec 2, 2011

Only if there are no spaces, then it extends the table so you have to scroll sideways. Otherwise the text wraps, and looks ok.

@jdespatis
Copy link
Contributor Author

André, are you ok with the varchar(255) for the name ?

(Btw, just a reminder: I guess that whenever a change is performed to the ezscriptmonitor sql tables, an ALTER sql is needed into the update/database/(mysql|postgresql)/4.7/dbupdate-4.6.0-to-4.7.0.sql and in unstable also)

@andrerom
Copy link
Contributor

Yes

( correct, will you add it? We should also remember oracle. )

@jdespatis
Copy link
Contributor Author

Yes I'll add it (for mysql at least), maybe some help would be needed for postgresql and oracle :)
I prepare the pull right now

@jdespatis
Copy link
Contributor Author

Here we are: ezsystems/ezpublish-legacy#251

(postgresql is lacking, please André if you have some time/skills on it, consider add another commit to complete it)

I've not considered oracle at all, as the latter is not listed in update/database (it would be nice of course to add it, and add specific script for each upgrade)

have a nice we!

@jdespatis
Copy link
Contributor Author

Oh sorry, the change on database must be bundled in ezscriptmonitor so
=> @andrerom can you tell me where I have to create this dbupdate.sql script ?

@glye
Copy link
Member

glye commented Dec 12, 2011

This doesn't seem to be set in stone. The only other example I found is in eZ OE:
update/database/5.0/dbupdate-5.0.0-to-5.0.1.sql
and this doesn't specify database brand.

I'd say the path you used looks good:
update/database/mysql/4.7/unstable/dbupdate-4.6.0-to-4.7.0alpha1.sql
however the version is wrong when this is in the extension. In eZ Publish 4.7, ezscriptmonitor will be in version 1.4.0, so please use that:
update/database/mysql/1.4/unstable/dbupdate-1.3.0-to-1.4.0alpha1.sql

@andrerom please confirm.

@jdespatis
Copy link
Contributor Author

@glye thanks for the link, it seems good, I've added the file so (postgresql is lacking, feel free to add it)

@andrerom please confirm if the logic is the good one

@andrerom
Copy link
Contributor

confirmed, over!

@ghost ghost assigned glye Jan 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants