Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix PEAR Date bug and allow REST API to set campaign expire_time to never expire #768

Closed
wants to merge 2 commits into from

Conversation

@mvdklip
Copy link
Contributor

commented Oct 14, 2016

While working on a request for adding the ability to set the expire time of a campaing to never expire I found two components in the main ad server which prohibited this. While examining the web ui I saw that it is passing a NULL to set the expire time to never expire, so I set out to make the REST API do the same. Internally this NULL is converted into a PEAR Date which then becomes an (invalid) date in the year zero. However due to a bug in PEAR Date the date got mangled during conversion to UTC. Fixing that is one part of this PR. The other part is handling the year zero date inside the campaign dll and converting it back into a proper SQL NULL.

mvdklip added 2 commits Oct 12, 2016
Fixed bug in PEAR Date library
This prevented years < 1000 from properly being converted to UTC
Extended Campaign Dll to allow setting the expire_time to 'never expire'
This is done by passing a year 0 (invalid gregorian) date in the campaign which
gets converted to a DO NULL value. The Revive Adserver REST API is actually
already passing such a value, but mentioned Dll was not handling this case.

@mbeccati mbeccati self-assigned this Oct 14, 2016

@mbeccati mbeccati added the Bug label Oct 14, 2016

@mbeccati mbeccati added this to the v4.0.1 milestone Oct 14, 2016

@Quix0r

This comment has been minimized.

Copy link

commented Jan 9, 2017

Na, changing external libraries (PEAR is not from revive) make it later maybe impossible to update the lib (generally said). So instead maybe report this change upstream (to PEAR, not here).

@mvdklip

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2017

@Quix0r

This comment has been minimized.

Copy link

commented Jan 10, 2017

When it has been abandoned, go the usual way of forking it and then fix it there. If you only fix it here, the fix won't likely spread and remains unfixed in many other places. If they find it there, too. I think they will be glad someone came up with a fixed version.

Or also abandon it here, too and switch to a successor project (for PEAR I currently don't know).

I did here the same with with PHPExcel (it is not abandoned, but deprecated) but the successor (PHPSpreadsheet) is currently a bit unstable and not suitable for daily usage. So I forked both projects, fixed some stuff in PHPExcel and surely ported it to PHPSpreadsheet to help out both sides. :-)

@mbeccati

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

FYI, the PEAR packages included with Revive have been forks since a long time as some of our patches and fixes hadn't been accepted upstream.

@mbeccati

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

I've reviewed the PR and after lots of thinking, I don't feel this is the right approach: an invalid date is a poor substitute for a proper NULL value. XML-RPC cannot represent the NULL type, but there is an extension for it that adds . Going from memory, the PEAR client/server library we use doesn't support it, but ultimately it should not be extremely hard to go that route.

@mbeccati mbeccati closed this Jan 12, 2017

@mbeccati mbeccati removed this from the v4.0.1 milestone Jan 12, 2017

@chasebolt

This comment has been minimized.

Copy link

commented May 29, 2018

is there any plans in the works for fixing this? this is a long lived bug which affects the REST API.

@erikgeurts

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2018

Revive Adserver is capable of setting the end date of a campaign to 'don't expire' by setting the campaign's end date to a 'null' value in the 'expire_time' column of the rv_campaigns table. This is possible because the core software doesn't make use of the API.

As explained by @mbeccati in his earlier comment above, this issue originates from the fact that the XML-RPC specifications do not have any way to represent a NULL value. This is unfortunate, but not something we can easily fix. I see Matteo's point that passing an invalid date is not a good alternative for passing a real 'null' value. That still leaves us with the question of how to handle this from an API.

Developers using the XML-RPC API or the REST API (full disclosure: the latter is a commercial, third party product sold by my company) and in need of a way to get around this issue, could consider implementing the work-around provided by @mvdklip in his commits from October 12, 2016. Even though it may not be the most pure approach, it does work and will enable developers to proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.